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

Warn when migration was inferred as non-reversible and user set rever… #3597

Conversation

kyle-leonhard
Copy link

@kyle-leonhard kyle-leonhard commented Nov 6, 2024

This PR prints a warning when trying to add a reversible migration on top of non-reversible migrations. This behavior was added along with the PR that added inference: #2664.

First time contributor here - I read the code and it's not clear why reversible migrations can't be stacked on top of non-reversible ones. Obviously this would limit which versions one could revert, which seems fine.

I'm in the spot where I was lazy in the beginning of a project and used non-reversible migrations. But now I'd like to start using reversible ones.

Does your PR solve an issue?

Running sqlx migrate add -r something after previously adding non-reversible migrations ignores the -r flag and creates a non-reversible migration. I was confused for a bit.

#3598

@abonander
Copy link
Collaborator

This is a bug. The command-line flag should always override the inferred behavior.

I have a fix in #3383 but it's going to take me some time yet to get that over the finish line.

I'm going to close this since it's the wrong approach, but please feel free to open a new PR with the bugfix.

@kyle-leonhard
Copy link
Author

This is a bug. The command-line flag should always override the inferred behavior.

I have a fix in #3383 but it's going to take me some time yet to get that over the finish line.

I'm going to close this since it's the wrong approach, but please feel free to open a new PR with the bugfix.

Thank you very much, @abonander

@kyle-leonhard kyle-leonhard deleted the warn-on-mixing-reversible branch November 27, 2024 23:13
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.

2 participants