-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix/3717 filter table on diesel migration --diff-schema
#3725
Conversation
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 working on this. The implementation looks really good 👍 I have one minor question otherwise this is fine from my side.
I've rebased this branch to pull in the changes from #3729 to fix the broken tests. |
diesel_cli/src/cli.rs
Outdated
.index(2) | ||
.num_args(1..) | ||
.action(clap::ArgAction::Append) | ||
.help("Table names to filter (default only-tables if not empty)."), |
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.
@weiznich would you like me to change this to:
.help("Table names to filter (default only-tables if not empty)."), | |
.help("Table names to filter"), |
Because as far as I can see, filtering so far has only worked by specifying -o
or -e
the change would also be made for print-schema below
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'm in favor of this change (also for the print-schema) part.
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.
Sorry for the delay I've just made this change. I think this should be good to go now, as long as you're happy with it.
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.
No worries from taking some time. We all work on this in our free time, so it's fine to take as much time as needed. There is always real life, which is more important than this.
Thanks for rebasing this!, I've left a couple comments and will set ready for review. Thanks for approving already too 😄 |
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 👍
Fix/3717 filter table on `diesel migration --diff-schema`
Fixes: #3717
I have made changes to allow
diesel migration --diff-schema
to accept filter args matchingdiesel print-schema
I came across a few issues while working on this:
filter
attribute fromPrintSchema
toConfig
without a breaking changetable-names
arg seems incorrect to me, It currently states(default only-tables if not empty).
But I'm pretty sure it defaults toNone
currentlyTODO:
I'll add some more info to this PR description before requesting review