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

Rewrite argument parsing to use Clap's derives #2962

Merged
merged 2 commits into from Jul 10, 2022

Conversation

shssoichiro
Copy link
Collaborator

This is something that has been on the radar for a while, but the recent
deprecation of everything from Clap 3.1 in Clap 3.2 has pushed our hand
a bit to get it done now with the new best practices from Clap 3.2.

@lu-zero
Copy link
Collaborator

lu-zero commented Jun 25, 2022

Looks prettier by far :) Thank you!

@redzic
Copy link
Collaborator

redzic commented Jun 25, 2022

Changing all occurrences of the speed preset from a usize to a u8 seems to fix the CI (and also local) failures.

@shssoichiro
Copy link
Collaborator Author

I wonder if anyone is aware of an alternative to clap_complete that supports generating completions from the derive API?

@redzic
Copy link
Collaborator

redzic commented Jun 26, 2022

It looks like there is an example for generating completions with the derive API: https://github.com/clap-rs/clap/blob/master/clap_complete/examples/completion-derive.rs

@shssoichiro shssoichiro force-pushed the clap-derive branch 4 times, most recently from 7714f6d to a0f9c0f Compare June 27, 2022 21:18
@coveralls
Copy link
Collaborator

coveralls commented Jun 27, 2022

Coverage Status

Coverage decreased (-0.2%) to 84.964% when pulling 0222e23 on shssoichiro:clap-derive into a8d1e46 on xiph:master.

pub bitrate: Option<i32>,
/// Speed level (0 is best quality, 10 is fastest).
/// Speeds 10 and 0 are extremes and are generally not recommended.
#[clap(long, short, value_parser = clap::value_parser!(u8).range(0..=10), default_value_t = 6, help_heading = "ENCODE SETTINGS", long_help = build_speed_long_help())]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[clap(long, short, value_parser = clap::value_parser!(u8).range(0..=10), default_value_t = 6, help_heading = "ENCODE SETTINGS", long_help = build_speed_long_help())]
#[clap(long, short, value_parser = clap::value_parser!(u8).range(0..=10), default_value_t = 6, help_heading = "ENCODE SETTINGS", long_help = build_speed_long_help)]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting. It actually failed to compile for me when I omitted the () and succeeded without. 😕 I'll try again and see if I'm crazy or my machine is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it turns out it's unrelated to the parens, it's complaining because with the serialize feature on, the method returns Option<String> and it wants an Option<&str>. Which, I'm not really sure how to make it an Option<&str> because of lifetimes. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fairly annoying since it would require a lazy_static.

Copy link
Collaborator

Choose a reason for hiding this comment

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

or once_cell as @Luni-4 pointed out :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, it seems lazy_static is used more for this reason

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, sorry, I meant to push up this change, this week has just been quite busy with IRL job. 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be working now. At least, it worked locally 😅

This is something that has been on the radar for a while, but the recent
deprecation of everything from Clap 3.1 in Clap 3.2 has pushed our hand
a bit to get it done now with the new best practices from Clap 3.2.
Copy link
Collaborator

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

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

Thank you!

@shssoichiro
Copy link
Collaborator Author

shssoichiro commented Jul 9, 2022

Not really sure where these new clippy errors came from... but I guess I have to fix them. 😅 They seem unrelated to my changes.

Edit: Oh, a new stable Rust was released on June 30 and somehow I missed it.

@shssoichiro
Copy link
Collaborator Author

Welp.

rust-lang/rust-clippy#9142

@shssoichiro shssoichiro merged commit 182bbff into xiph:master Jul 10, 2022
@shssoichiro shssoichiro deleted the clap-derive branch July 10, 2022 16:50
@barrbrain barrbrain added this to Done in Release 0.6 Sep 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants