Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

CLI: Reject invalid argument values rather than ignore them #6723

Merged
merged 2 commits into from
Oct 13, 2017

Conversation

axelchalon
Copy link
Contributor

Previously (since the CLI port to clap), passing invalid values (e.g. providing a string value to an argument that expects a u32) was failing silently and was processed just like if the argument hadn't been entered.

Now, Parity explicitly refuses to launch if Clap cannot parse the value into the expected type.

$ parity --cache=asd                            
error: Invalid value: The argument 'asd' isn't a valid value

@axelchalon axelchalon added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 12, 2017
@axelchalon axelchalon added this to the 1.8 milestone Oct 12, 2017
Err(clap_error @ ClapError { kind: ClapErrorKind::ValueValidation, .. }) => {
return Err(clap_error);
},
_ => $e
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be better to unwrap the value here rather then calling ok() later.

		match $e {
 			Err(clap_error @ ClapError { kind: ClapErrorKind::ValueValidation, .. }) => {
 				return Err(clap_error);
 			},
 			Ok(e) => e
         }

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason to ignore errors other than ClapErrorKind::ValueValidation btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arkpar the clap macro value_t! (whose result I feed into the return_if_parse_error! macro) yields a Result ; the error can be either ClapErrorKind::ArgumentNotFound or ClapErrorKind::ValueValidation. If it's ArgumentNotFound then the value of the argument in the struct should be None.

@debris I thought about it but figured it'd be better if the macro didn't convert to Option for separation of concern, but I agree it'd be better for DRY. $e can be either ClapErrorKind::ValueValidation error, or ClapErrorKind::ArgumentNotFound error, or Ok(...) ; in the two latter cases this is normal behaviour and we want to convert it to Option; I guess the following would be good?

match $e {
 			Err(clap_error @ ClapError { kind: ClapErrorKind::ValueValidation, .. }) => {
 				return Err(clap_error);
 			},
 			_ => $e.ok()
         }

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, just describe it in the comment ;)

@debris debris added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 12, 2017
@5chdn 5chdn mentioned this pull request Oct 12, 2017
67 tasks
@5chdn 5chdn added B0-patch P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible. labels Oct 12, 2017
@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Oct 13, 2017
@arkpar arkpar merged commit d77daba into master Oct 13, 2017
@arkpar arkpar deleted the cli-invalidvalues branch October 13, 2017 10:21
arkpar pushed a commit that referenced this pull request Oct 13, 2017
* CLI: Reject invalid argument values rather than ignore them

* Fix grumbles
arkpar added a commit that referenced this pull request Oct 13, 2017
…6747)

* CLI: Reject invalid argument values rather than ignore them

* Fix grumbles
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants