-
Notifications
You must be signed in to change notification settings - Fork 372
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
feat: all in one migration command #3250
Conversation
🚀 Deployed on https://deploy-preview-3250--moor.netlify.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried about supporting multiple databases, but splitting generated migration assets into different directories is a clever solution.
The implementation looks good to me, some things I'm wondering from a DX perspective:
- Most users will have exactly one database - I think we should make that case a little easier by also allowing a simple
String
(as a file path) in thedatabases
option (or perhaps with a separatedatabase
key). - Maybe we could use the entries of that map to not only configure the name of the database (which doesn't appear to be considered for file paths) but also the location under
test/
where the default test should be added to? - I kind of view this as a "make the first migration tooling setup easier" command. In particular, I imagine that users will want to use the output of
writeGeneratedTest()
as a starting point to add own migration tests too. So maybe we shouldn't overwrite the file every time and instead only write it if it doesn't exist?
1 will require fighting with freezed. Not sure how to do that? sealed unions? |
We're using json serializable. I think you can keep the field for this and then use |
We must resolve #3219 (comment) |
65ef5b3
to
3a37d49
Compare
@simolus3 How do you suppose I write tests for all this. Most of this functionality is copied from |
@simolus3 I did not implement point 1 myself. Left for you to implement - it's likely quicker for you given your familiarity with json_serializable. Regarding point 3, we could put a function in the generated Overall, I this is coming together much quicker than I would like. Please try this out yourself in practice and Lmk what the user experience is like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good to me. Some bigger notes are:
- We should adopt this in the
example/migrations
project (if necessary, copy the existing schema dumps into the directory where this tool would expect them). Then let's add a section at the top of the readme showing how to use the new all-in-one command. We can keep the older commands as a reference for users who need manual control. - Since we expect that most users will want to run this command instead of the previous multiple invocations, let's highlight it more prominently. I think it deserves to be at the top of the migrations tooling page, mentioning that it does everything the other commands are doing in a single step, and that the other commands are then mostly intended for users needing more customization / integration into other tools.
- Instead of pointing to
--help
on the documentation website, I think the website should give an introduction into the command or at least point to the migrations example. The colorful usage output looks great, but users will probably expect this information on the site as well.
Agree, moved around some around of the docs. |
d06bc10
to
1d742e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work! LGTM.
I'll just try to get the CI fixed :D
Generates Dumped Schema, Tests and Steps with a single command.
Uses
build.yaml
config to define database and file paths.Fixes: #3216
This basically wraps
generate
,dump and
steps`.A small refactor is needed to make the code in each of those commands reusable by this new command.