tealdeer-rs / tealdeer

A very fast implementation of tldr in Rust.
https://tealdeer-rs.github.io/tealdeer/
Apache License 2.0
4.17k stars 123 forks source link

Simplify clap derive definition #221

Closed tranzystorekk closed 2 years ago

tranzystorekk commented 2 years ago

Highlights:

dbrgn commented 2 years ago

Make short and long attributes automatically parse field names

Hi, we discussed automatically parsed field names here: https://github.com/dbrgn/tealdeer/pull/108#discussion_r730452346

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

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

tranzystorekk commented 2 years ago

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

dbrgn commented 2 years ago

@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!