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

Codegen output to multiple files (one per table) #906 #1785

Merged
merged 1 commit into from
Mar 28, 2018

Conversation

Asamsig
Copy link
Contributor

@Asamsig Asamsig commented Sep 21, 2017

This is a continuation of pull request 987, solving issue #906

I've updated the code to work with 3.2.x, added multiple file out to the current test suites, i.e. CodeGeneratorRoundTripTest and CodeGeneratorAllTest. Everything should be working. I've added a new boolean to the run methods called outputToMultipleFiles, I've set the default value to false, it shouldn't break any existing behavior.

@Asamsig
Copy link
Contributor Author

Asamsig commented Oct 13, 2017

If anything is preventing this from being merged in I'll be happy to adjust it.

@hvesalai hvesalai added this to the Next feature release milestone Feb 28, 2018
@hvesalai hvesalai modified the milestones: Future feature release, 3.3 Mar 26, 2018
@hvesalai
Copy link
Member

@Asamsig still happy with this. If there are no objections, I could include this in 3.3

@Asamsig
Copy link
Contributor Author

Asamsig commented Mar 26, 2018

This should be good. Would be great to see in the next release. 👍

@hvesalai
Copy link
Member

hvesalai commented Mar 27, 2018

If so. My last request is that we do not merge all those commits separately, but do some cleanup.

I see two approaches and I let you choose 😉 :

  1. 🔫 ‼️ cowboy version = I squash-merge all the commits to one

  2. 🍷 git-connoisseur version = you masterfully rework (i.e. squash, reorder, what ever) the commits into a series of meaningful commits that I then merge with a merge commit

@octonato
Copy link
Contributor

I will argue that squash-merge is not really a cowboy version, but the best option we have to easy maintenance. If there is only one single commit it's very easy to backport changes by cherry-picking.

The common practice in many projects is to have a master branch where the PRs are sent to and one or more maintenance branches, for instance here we have now the 3.2.x branch. Whenever we have a fix that we want to release in the 3.2.x, we can cherry-pick it.

Back in the time, when #1274 was fixed, it was impossible to make a quick bug fixing release. The master had diverged too much from the maintenance branch and the master was not ready to be released. #1274 couldn't be integrated on the maintenance branch because it depended on other changes that were not cherry-pickable neither. As consequence, #1274 was fixed, but nobody could benefit from it until master was considered stable again.

The akka project have been using this technique successfully for quite some time.
Creating commits and Writing commit messages

The bottom line is: between a list of commits with some meaningful messages, that probably nobody will analize later on, and a cherry-pickable commit, there is much more value, from a maintenance point of view, to choose for the latter.

(my 2cc)

@hvesalai
Copy link
Member

@renatocaval hear hear! I agree with most of that. By option 2 I meant one, or possibly more, cherry-pickable commits. If it is going to be just one, then I can do it with squash merge, but if there are parts of this PR that could be used as independent commits, then I think more commits is better.

@Asamsig
Copy link
Contributor Author

Asamsig commented Mar 27, 2018

I don't think any of the seperate commits has any value, so we should probably just squash it all. I have time tomorrow to fix this.

@Asamsig Asamsig force-pushed the codegen/multiplefilesoutput branch 3 times, most recently from 4b0404f to a05d562 Compare March 28, 2018 11:22
@Asamsig
Copy link
Contributor Author

Asamsig commented Mar 28, 2018

I've now squashed all the commits into one commit, I ran into a few problems, but I think I resolved everything correctly, the tests look fine on my machine, so we'll see what the CI says. :)

@hvesalai
Copy link
Member

can you document the problems, please

@Asamsig
Copy link
Contributor Author

Asamsig commented Mar 28, 2018

It was just git problems, I couldn't squash automatically, so I had to resolve a few conflicts and such. I ran the tests locally, and everything seems in order.

@hvesalai
Copy link
Member

Looks good! What about documentation? Does that need to be updated?

@hvesalai
Copy link
Member

I.e. anything in here that needs to change: http://slick.lightbend.com/doc/3.2.3/code-generation.html

@Asamsig Asamsig force-pushed the codegen/multiplefilesoutput branch from a05d562 to 4ba463f Compare March 28, 2018 14:55
@Asamsig
Copy link
Contributor Author

Asamsig commented Mar 28, 2018

I've added a little information to the documentation, describing the new parameter and the effects of it.

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