search

lastland / scala-forklift

Type-safe data migration tool for Slick, Git and beyond.
Other
188 stars 31 forks source link

Make updateOp happen first in migrateOp #34

Closed markyao6275 closed 6 years ago

markyao6275 commented 7 years ago

In the command list, the description of migrate is: migrate automatically fetch migrations, preview and apply them

Thus, I believe that migrateOp should fetch migrations (i.e. call updateOp) first, not last.

CLAassistant commented 7 years ago

CLA assistant check
All committers have signed the CLA.

lastland commented 7 years ago

Hi @markyao6275, thanks for the PR!

I'm a little bit uncertain about this change. I intentionally put updateOp in the last place so that program will almost immediately terminate after this operation. The reason for that is that this operation will change the source code, and we want to run all subsequent operations on the program compiled from the updated source code. I'm not certain whether this program still behaves as expected with this PR.

Is there a particular reason you want updateOp first in migrateOp?

markyao6275 commented 7 years ago

Hi @lastland. Thanks for the reply!

I don't quite understand your point about why this program will not behave as expected after making updateOp first.

The reason I want this change is that the help text for migrateOp says:

migrate automatically fetch migrations, preview and apply them

So if I define a new migrations M2 in src_migrations and run migrate, I expect the new migration to be added to Summary.scala, preview-ed and then applied. Currently, this is not what happens. What happens is that we preview no migrations, apply no migrations, and then finally, update Summary.scala to include M2. I have to run the migrate command again (or minimally, the apply command) to actually execute my migration.

I would like to be able to just apply my newly added migrations in one command, which is why I think that migrate should update first.

Let me know if this is not clear.

lastland commented 7 years ago

I will explain more details below, but here's a short answer: If you are just looking for one single command to apply your newly added migrations, ~mg migrate is the way to go.

And my complete answer:

Currently, this is not what happens. What happens is that we preview no migrations, apply no migrations, and then finally, update Summary.scala to include M2. I have to run the migrate command again (or minimally, the apply command) to actually execute my migration.

This is true. However, you will not get the desired effect by moving updateOp forward, and here is why: As you know, updateOp will write to the summary. And the summary itself is a Scala file. The changed summary file will not take effect until it has been recompiled. That means previewOp, etc. will not be executed until the summary has been recompiled because the summary is still empty! Maybe there are ways to hot-patch Scala code, but we choose to use a simple trick in Forklift, that is, just to recompile the whole project.

Of course, the consequence is that you will need to run mg migrate again. However, Forklift provides a variantion of that command, i.e. ~mg migrate, to help you do all these in one command: what it does is running mg migrate again and again automatically when some changes to your source code has been detected.

Does this make sense to you?

lastland commented 6 years ago

I'm closing this. Let me know if you still have any questions. :)