Switch CLI arg parsing from clap to lexopt
#599
Conversation
|
Consider https://github.com/blyxxyz/lexopt as well it is "more correct" across some dimensions, while being similarly lean. |
CosmicHorrorDev
changed the title
clap to pico-argsclap to lexopt
|
You had me at more correct! Thanks for the heads up! The error handling was a little more fiddly, but overall it was pretty easy to switch over to since I could keep a lot of the existing work (help generation, tests, etc) |
|
Note that #602 is going to require additional support. |
|
Thanks for pointing that out! It looks like porting the cli work for that should be pretty trivial too, so I'm fine with having either PR merged first |
|
anything I can do to help get this one merged? |
|
Thanks for asking @marcograss I've mostly fixed merge conflicts locally. It just needs a little more work. I've got the next week off work to visit family, so I can probably find some time to finish up my changes |
|
Is this ready or is there more work that needs to be done here? |
|
Unless I'm forgetting something this should be ready for review whenever that winds up happening |
…2685) After this, we are still waiting for `indexmap` & `dashmap` to provide the new `hashbrown` to reduce duplicate dependencies, and for `criterion` to remove `clap` and release a new version. We're also waiting for a new version of `icu_datagen` that bumps the `zip` dependency to avoid a potential vulnerability. Ideally, they would also bump the `simple_logger` dependency, which is pretty outdated. In any case, `simple_logger` still uses an unmaintained `atty` dependency. Relevant issues: - xacrimon/dashmap#250 - unicode-org/icu4x#3150 - bheisler/criterion.rs#599 - borntyping/rust-simple_logger#74
|
This is quite the large change. Is anyone up for reviewing it? |
| Self::DisplayHelp => f.write_str("Signals to display help"), | ||
| Self::DisplayVersion => f.write_str("Signals to display version"), |
There was a problem hiding this comment.
Those cases aren't meant to happen, so I would suggest moving DisplayHelp and DisplayVersion into another enum and change try_parse_args' signature:
enum ParseResult {
Ok(Args),
DisplayHelp,
DisplayVersion,
Err(Error),
}Since we're not doing anything fancy like ?, the Err(_) variant can either be inside ParseResult or in a wrapping Result.
There was a problem hiding this comment.
Hi, sorry for the duplicate notification, I missclicked on the single-comment button ^^
In the end I don't have much to say on this PR. It looks pretty good to me.
It is possible to move configure_from_args into the cli module as well (even if cli is private), so that it's easier to feature-gate and trim more deps.
| Ok(s) => s.parse().map_err(|_| Error::InvalidFlagValue(flag, arg)), | ||
| Err(_) => Err(Error::InvalidFlagValue(flag, arg)), | ||
| }, | ||
| Err(_) => Err(Error::FlagMissingValue(flag)), |
There was a problem hiding this comment.
lexopt::Error already has a missing-flag variant, which is what is returned when Parser::value returns an error.
This is one less variant to maintain. :)
| use lexopt::prelude::*; | ||
|
|
||
| let mut args = Args::default(); | ||
| let mut parser = lexopt::Parser::from_iter(raw_args.into_iter()); |
There was a problem hiding this comment.
You can have raw_args be of type impl Iterator<Item=OsString>, then you don't need to into_iter() nor to collect the args_os iterator above
| Self::DisplayHelp => f.write_str("Signals to display help"), | ||
| Self::DisplayVersion => f.write_str("Signals to display version"), | ||
| Self::LexOpt(e) => write!(f, "Error parsing args: {}", e), | ||
| Self::UnexpectedArg(args) => write!(f, "Extra args that weren't processed: {:?}", args), |
There was a problem hiding this comment.
I think this message could be improved.
This is triggered on the first unexpected flag or additional value (filter). I think the two cases should be separate and have different error messages: one for flags "unexpected option {}" and one for duplicate filter "unexpected extra filter {}".
Also, I don't know if this is on purpose but the arg is doubly formatted with its Debug impl, which renders a lot of escaped quotation marks.
Also, it's sad that lexopt::Arg does not implement Display.
|
I'm really busy with other work atm, and don't feel up to going back to this PR to try and get it merged (fun fact: since I started on this PR I both broke my wrist, and had it fully heal as of many months ago) Someone else is free to pick up the torch if they want. Sorry to all the people who left feedback in various ways |
Resolves #596
(pinging @lemmih I'm targeting
version-0.4in case you want to include this in version 0.4)This swaps the CLI arg parsing from
claptopico-args. Doing so drops the number of dependencies by 6 (and all ofpico-argsitself is just over 800 lines comments and all)I think the organization speaks for itself. I was going to add more tests along with doing some more manual tests, but I may have sprained my wrist today, so I'm going to go easy on the typing 😅