dbsrgits / sql-translator

SQL::Translator (SQLFairy)
http://sqlfairy.sourceforge.net/
82 stars 91 forks source link

Oracle producer add missing functions #143

Closed hazardv closed 1 year ago

hazardv commented 2 years ago

Adds functionality to Producer/Oracle.pm to:

Related tests have been added to prove changes and all existing tests pass. Automatic creation of trigger to insert sysdate to all timestamp fields has been commented out. That should not be done automatically.

Changes were modeled after the structure of Producer/MySQL.pm.

This branch also includes changes from pull request #142

hazardv commented 2 years ago

@mohawk2 The code you reviewed was from a few commits back. This is the first time I am attempting to contribute to a project like this so I am betting that I messed a few things up.

I have been using DBIx::Class and DBIx::Class::Schema::Versioned for another project and while working on that I discovered functionality that was missing from the Oracle producer. I added in the missing functionality and submitted the pull request. I continued with my project, made changes to the data model, attempted to generate the DDL, and discovered a few more things missing from the Oracle producer. So I added those things into the same branch because it seemed reasonable and pushed the changes. I did that whole process one more time and now I am like 97% sure that all of the missing functionality has been implemented and I won't make any more changes to the branch.

Please check out the latest commit b14da74 and let me know if you see any issues.

mohawk2 commented 1 year ago

Thanks for the updates.

The current list of commits sort of meanders, which isn't a criticism, but does highlight a possible process-improvement, especially to help reviewers. Since this is on a branch belonging to you, it's safe to force-push it. The way I work is to end up with the branch I'm making having a series of small commits, each of which meaningfully transforms the starting code towards the desired end-state. This often involves squashing together (using rebase "fixup" command) previous versions of a change, with a correction. This means the final list of commits doesn't have things like commented code, then removal of that (as here). Having adjusted the list of commits to a new one, you'd then need to force-push it.

The alternative (which isn't fatal) is that this set of commits will either (if merged in as-is) be slightly untidy for future people looking to see why a piece of code is as it is, or (if squash-merged) a single quite large commit which can take longer to comprehend.

Tagging in @rabbiveesh.

hazardv commented 1 year ago

@mohawk2 Thank you for the pointers. I will try to clean up my commits to be more concise in the future. I found one last issue with altering constraints for Oracle while working on my personal project (dropping an unnamed primary key constraint) and have pushed the change to this branch. At this point, I have personally tested all of the constraint alteration functionality in my other project (as well as adding tests for this project) and am confident in them.

hazardv commented 1 year ago

@rabbiveesh after reverting the change in Diff.pm (and fixing one of my test plans) all of the tests are passing on my box except for t/60roundtrip.t. It is looking for t/data/roundtrip_autogen.yaml which I am unable to locate in my branch or in the current project master.

rabbiveesh commented 1 year ago

Yeah, to get the roundtrip test working you need some script. You can run Makefile.PL, and ~then run make test~(you only need to run Makefile.PL), it should initialize whatever you need.

hazardv commented 1 year ago

@rabbiveesh Sorry about that, I did not realize I had introduced an issue. With the latest commit (74c0312) the tests all pass on my box (including roundtrip).

hazardv commented 1 year ago

@rabbiveesh I added them following how they were used in the MySQL producer, tagged them with "ORA" at the start, and included them sparingly figuring they could be useful for someone debugging their code in the future. Since they only generate output if the user specifies $DEBUG = 1 I didn't think they hurt anything to be there but if you want them removed I will remove them.

hazardv commented 1 year ago

Hey @mohawk2, can you please take a look at this when you get a minute?

hazardv commented 1 year ago

@mohawk2 Happy New Year!!!

hazardv commented 1 year ago

I'm a bit surprised by the tests being removed from the XML-related tests. If @rabbiveesh is content it's for a good reason then so am I. Otherwise, all looks fine.

@mohawk2 The only things I removed from the XML tests were the checks looking for the triggers that previously got created for all timestamp fields. Since there is no reason to create that trigger for all timestamp fields I commented out the code in commit https://github.com/dbsrgits/sql-translator/pull/143/commits/ab42ef849111e4091ccae0981bb4241d60c15146. I then removed the commented-out code in commit https://github.com/dbsrgits/sql-translator/pull/143/commits/b14da74a5bb7e2e342e2651c6abfdf656e153128.

rabbiveesh commented 1 year ago

sorry for the delay on this; we're good to go