Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NuoDB Producer #71

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

NuoDB Producer #71

wants to merge 15 commits into from

Conversation

jazd
Copy link

@jazd jazd commented Sep 28, 2015

Create NuoDB DDL from XML-SQLFairy, NuoDB Producer.

It works good enough for my current use, but I am willing to maintain and continue to submit improvements.

@ribasushi
Copy link
Contributor

Hello @jazd, sorry for not getting to you earlier. The patch looks relatively good (your starting point (...DB2.pm) isn't great, but that isn't your fault in any way).

Do you happen to have any plans on making a ::Parser counterpart to this (and checking whether it properly roundtrips via t/60roundtrip.t)? If not I suppose we will merge the thing as-is, and call it good. But if you were planning on adding a grammar to this, it would be much much better to get the changes in as a "package", as this will undoubtedly catch yet-unknown issues.

@jazd
Copy link
Author

jazd commented Nov 12, 2015

@ribasushi I do not have any plans to produce a ::Parser. If one is needed by others in the future, I am willing to work on it. All of my schemata start out in XML-SQLFairy format :)

@jazd
Copy link
Author

jazd commented Nov 24, 2015

@ribasushi please merge as-is.

@jazd
Copy link
Author

jazd commented Feb 29, 2016

I am referencing sql-translator in my https://github.com/jazd/Business project. I made it public today. sql-translator is required to build the project from source.

@ribasushi
Copy link
Contributor

Hello @jazd
Apologies for the slow response on this.

Given that SQLT is generally moving in the direction of "less kitchen sinks", and an upcoming release will (try to) make this distinction more prominent, I think it would be more conductive to release this producer as a standalone CPAN module with a dependency on SQL::Translator.

This way you both get to develop it on an independent schedule, and the sprawling growth of the core SQLT dist won't continue.

I am not saying strictly "this won't merge", nor am I closing this ticket yet. I want to hear your thoughts on that first.

@jazd
Copy link
Author

jazd commented Mar 11, 2016

@ribasushi,

I would rather this producer be included before you make such a move. If you decide to pull out a group of Producers and Parsers at a later date, we can revisit if NuoDB should be on that list.

There are so few SQL database servers in the world, are you sure this move is necessary? Also, is sqlt useful on its own, without the producers/parsers?

@ribasushi
Copy link
Contributor

Fair enough (and this is why I wanted to wait to hear from you). I will do a final review and add, hopefully as soon as this weekend.

@jazd
Copy link
Author

jazd commented Mar 12, 2016

Thank you :)
I have now released my schema with a NuoDB target https://github.com/jazd/Business/releases/tag/0.0.2

@jazd
Copy link
Author

jazd commented Sep 23, 2017

The travis-ci failures are due to build script issues in Perl 5.8 and Perl blead system installation and setup prior to running any tests.

@ribasushi Hope all else is well. Please merge as-is.

@ribasushi
Copy link
Contributor

@jazd Hi!

A series of not-so-recent events are preventing me from using my PAUSE account, so I had to suspend work on everything I used to maintain.

It seems like a resolution is finally within reach, but even when everything is settled it will take a while for me to unwind the piled up todo-list. In the meantime all I can suggest is contacting the current DBIC/SQLT/SQLA management on IRC.

I am sorry your contribution got caught up in this policy dispute. Hoping the freeze on my side won't last too long into the future.

Cheers!

@jazd
Copy link
Author

jazd commented Sep 23, 2017

@ribasushi Hello!
Thank you for the update. I am not in a huge hurry, so, for now, I will wait it out here with you.
Let me know if I can help with anything.
-Steve

@jazd
Copy link
Author

jazd commented Apr 6, 2019

@ribasushi can someone please re-run the build for this PR?
The original #230 failed due to what looked like a tool failure.

@codecov
Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #71 into master will increase coverage by 0.29%.
The diff coverage is 79.61%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #71      +/-   ##
=========================================
+ Coverage    21.5%   21.8%   +0.29%     
=========================================
  Files          71      72       +1     
  Lines       19914   20017     +103     
  Branches     5728    5751      +23     
=========================================
+ Hits         4283    4365      +82     
- Misses      14922   14933      +11     
- Partials      709     719      +10
Impacted Files Coverage Δ
lib/SQL/Translator/Producer/NuoDB.pm 79.61% <79.61%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86c68a0...2767a07. Read the comment docs.

@jazd
Copy link
Author

jazd commented Apr 7, 2019

@ilmari would it be possible to accept and merge my pull request?

@shadowcat-mst
Copy link
Member

@jazd the database looks really interesting but I'm not sure why you were being strung along with "maybe we'll merge it" when so far as I can tell this would 100% work fine as a standalone cpan module and then you wouldn't have to wait for us.

Is there a reason you wanted it in core rather than installable? If not, can I help you figure out how to get it onto CPAN in an orderly fashion?

@jazd
Copy link
Author

jazd commented Apr 6, 2020

@shadowcat-mst sure, I would appreciate assistance in making this installable if this will never be merged. I am willing to wait longer ;-)

@shadowcat-mdk
Copy link

@jazd Maybe you want to tag @shadowcat-mst in that reply and not me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants