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
Run a range of migrations #32
Conversation
It might be worth describing the argument information in README
|
52145e0
to
aca46f0
Compare
I've added unit tests to validate the order, however the status.json update itself, in my opinion, doesn't require more coverage. I have similar feelings about the README edits, I've updated it a bit and added an example into the help section, but the new concept of running migrations from a specified range nicely complements the current implementation. It modifies the status.json in the same way, it just writes each migration to status.json to avoid running it again. I've also added a validity check for the migration order, the current implementation already checks for duplicates, so I think it will be possible to close #33 as well. I don't want to check for example that there is no gap in the migration order, it would make deleting migrations unnecessarily difficult. I will add an example of running a range of migrations to the boilerplate as well |
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.
Overall seems OK.
Just a couple of notes -> none of them is super important, but the DOM in tsconfig.json
src/cmds/migration/run.ts
Outdated
const from = Number(match[1]); | ||
const to = Number(match[2]); | ||
|
||
return from <= to ? [from, to] : null; |
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 might return
{
from,
to
}
object
Just for readability.
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've introduced new IRange interface, thanks for the tip
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.
LGTM - the if/else forest in run.ts
might be refactored, but this is not a part of the task and it is still fairly readable.
Motivation
Which issue does this fix? Fixes
issue
Checklist
How to test
Try to run a range of migrations