-
Notifications
You must be signed in to change notification settings - Fork 6
Make Options::apply fallible #113
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
Conversation
500a487 to
c711166
Compare
|
Changes since initial push:
|
c711166 to
b2e8f73
Compare
|
Changes since last push: |
b2e8f73 to
acb3f57
Compare
|
Changes since last push:
|
acb3f57 to
cf04aea
Compare
|
Changes since last push:
|
cf04aea to
e2cdf41
Compare
tertsdiepraam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! My only real issue is with the Value thing I commented on. Once that's resolved: LGTM!
tests/coreutils/date.rs
Outdated
| enum Iso8601Format { | ||
| #[default] | ||
| #[value("date")] | ||
| // TODO: Express the concept "accepts prefixes" more nicely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I built this into Value already? It's working here for example:
uutils-args/tests/coreutils/ls.rs
Line 480 in a3ecc62
| let (s, _operands) = Settings::default().parse(["ls", "--format=acr"]).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! Fixed
|
|
||
| impl Options<Arg> for Settings { | ||
| fn apply(&mut self, arg: Arg) -> Result<(), uutils_args::Error> { | ||
| if self.chosen_format != Format::Unspecified { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I take a bit of issue with this example, because I don't like how Unspecified is a special thing that only exists for parsing. Essentially, it comes down to this: #11.
BUT! I don't care enough to block this. We can improve this later and we should move towards shipping this crate.
e2cdf41 to
66e1992
Compare
|
Changes since last push:
The clippy failure will be fixed by #125. Sorry, didn't see that it's my turn! |
Also, exhausting tests.
This causes CI failures, so let's fix this.
66e1992 to
e3f0b52
Compare
This PR shows one way we could make
Options::applyfallible.This feature trivially enables error prioritization, e.g. raising different errors for
--a=invalid --b=invalidand--b=invalid --a=invalid, which is what the GNU tools do.Closes #112