Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add --strict flag #441
Add --strict flag #441
Changes from 6 commits
eb36c90
68da288
9822a96
d278d6a
dc0e0a1
f912ddf
5b890ff
d9d7ce2
b9e0bdf
d0ded5d
311dc19
d20cf09
cd88906
3323fa5
d896ce2
4b85b89
5d95143
c216c93
9949e88
02d2d7b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is not the right place for this check to live.
FindMigrations()
should not fail regardless of what migrations have or haven't been applied.The check should be implemented in the
Migrate()
functionThere 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.
Without an additional db query the check needs access to a list of applied migrations to find the applied migration with the highest version number.
FindMigrations()
already queries those to check if a migration is already applied. There are several ways to do the check outside ofFindMigrations()
but I did not want to make too many unrelated changes which might be in conflict with other PRs, e.g. changing the return values ofFindMigrations()
would be in conflict with PR #438.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 put the strict version check in
Migrate()
and applied some small changes.FindMigrations()
already returns a sorted list of migrations so we only need to check the first pendig migration with the highest version number from all applied migrations.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.
similar to my other comment, I think this error message could be a bit more helpful. we can mirror whatever words are selected for the flag description.
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.
Changed the error message to the following: