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

Simplify clap derive definition #221

Merged
merged 4 commits into from
Oct 24, 2021

Conversation

tranzystorekk
Copy link
Contributor

@tranzystorekk tranzystorekk commented Oct 18, 2021

Highlights:

  • Make short and long attributes automatically parse field names
  • Add possible_values
  • Change render field type to PathBuf

@dbrgn
Copy link
Collaborator

dbrgn commented Oct 18, 2021

Make short and long attributes automatically parse field names

Hi, we discussed automatically parsed field names here: #108 (comment)

I'd tend towards keeping the explicit commands, mostly for consistency and because this way people can read/grep the commands without knowing how the clap auto-generated flag names work. For example, if someone wants to know how the clear-cache command works and searches the source code for that, there would be no instances that can be found.

Do you disagree with that?

@dbrgn
Copy link
Collaborator

dbrgn commented Oct 18, 2021

Add possible_values

Nice, I didn't know about this option. Unfortunately it causes a line wrap because it's a bit more verbose:

image

vs

image

@niklasmohrin what's your opinion, worth it or not? I don't think the possible_values features will bring any additional benefits, because the validation itself happens on the FromStr level (but I might be wrong about that). If it's purely convenience, I think I'd stick with the manually controlled variant, because it's more compact.

@tranzystorekk
Copy link
Contributor Author

Make short and long attributes automatically parse field names

Hi, we discussed automatically parsed field names here: #108 (comment)

I'd tend towards keeping the explicit commands, mostly for consistency and because this way people can read/grep the commands without knowing how the clap auto-generated flag names work. For example, if someone wants to know how the clear-cache command works and searches the source code for that, there would be no instances that can be found.

Do you disagree with that?

Something rubs me the wrong way when looking at the explicit version, but I cannot argue with your standpoint 😅

@tranzystorekk
Copy link
Contributor Author

Regarding possible_values, it might indeed be just a cosmetic change, unless FromStr impl changes to Err = Infallible, but then the validation would be split into two separate places in code...

One potential gain is if we switch to clap-generated completion which completes possible_values.

@dbrgn
Copy link
Collaborator

dbrgn commented Oct 18, 2021

One potential gain is if we switch to clap-generated completion which completes possible_values.

That's a very good point, I'm convinced 😄

src/main.rs Outdated Show resolved Hide resolved
@tranzystorekk
Copy link
Contributor Author

One more thought - should we leave a comment in code with a brief rationale for leaving flag names explicit?

@dbrgn
Copy link
Collaborator

dbrgn commented Oct 24, 2021

@tranzystorek-io thank you for the updates! Yes, such a comment would be helpful. Would you like to add one? Afterwards this should be ready to merge!

Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Thanks!

@dbrgn dbrgn merged commit 3beed4a into tealdeer-rs:master Oct 24, 2021
@tranzystorekk tranzystorekk deleted the simplify-clap branch October 24, 2021 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants