Skip to content

fix: Make sure table name handling is done by the crate instead of clap #92

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

Merged
merged 3 commits into from
Mar 30, 2025

Conversation

clflushopt
Copy link
Owner

@clflushopt clflushopt commented Mar 30, 2025

To allow supporting actual short hand we have to ensure that we use a custom TypedValueParser that is able to parse the refs correctly and align every abbrv with its long name and corresponding enum, one downside is that we lose some of the auto-derived nice things from clap like the list of options in the --help message due to a conflict from deriving ValueEnum vs implementing FromStr ourselves which is necessary to allow both abbrvs and long names.

I don't like this code because it's a lot of gymnastics for supporting command flags (I feel like clap is very much too powerful for our needs but maybe I am used to Go's stdlib provided argument handler).

There's also some non-trivial complexity in the implementation (side effect of how clap does things).

Closes #91

To allow supporting actual short hand we have to ensure that we use
a custom TypedValueParser that is able to parse the refs correctly
and align every abbrv with its long name and corresponding enum.

I don't like this code because it's a lot of gymnastics for supporting
command flags (I feel like clap is very much too powerful for our needs
but maybe I am used to Go's stdlib provided argument handler).

There's also some non-trivial complexity in the implementation (side
effect of how clap does things).
@clflushopt clflushopt requested a review from alamb March 30, 2025 03:31
Copy link
Collaborator

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @clflushopt !

I don't like this code because it's a lot of gymnastics for supporting command flags (I feel like clap is very much too powerful for our needs but maybe I am used to Go's stdlib provided argument handler).

There's also some non-trivial complexity in the implementation (side effect of how clap does things).

Thanks @clflushopt -- I didn't quite follow if you thought this PR was to complicated or if the clap implementation was too complicated. Is there anything you think we should be changing anything about this complexity?

@clflushopt
Copy link
Owner Author

@alamb The PR itself is not complicated, my comment was specifically about the ergonomics of clap but I guess it's powerful for a reason (less stuff for us to implement !).

@clflushopt clflushopt merged commit 59505b1 into main Mar 30, 2025
7 checks passed
@alamb alamb deleted the cl/fix/support-abbrv-tablenames branch March 30, 2025 19:07
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.

[FEATURE] Allow specifying tables by their prefixes similar to dbgen
2 participants