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

Add proper error message when using conflicting flags (e.g. --yes --no) #55

Closed
vrmiguel opened this issue Oct 1, 2021 · 4 comments · Fixed by #99
Closed

Add proper error message when using conflicting flags (e.g. --yes --no) #55

vrmiguel opened this issue Oct 1, 2021 · 4 comments · Fixed by #99
Labels
bug Something isn't working good first issue Good for newcomers high priority

Comments

@vrmiguel
Copy link
Member

vrmiguel commented Oct 1, 2021

No description provided.

@marcospb19 marcospb19 added bug Something isn't working enhancement New feature or request high priority and removed enhancement New feature or request labels Oct 1, 2021
@marcospb19
Copy link
Member

marcospb19 commented Oct 1, 2021

This should definitely be a FinalError crashing at src/cli.rs

ouch/src/cli.rs

Lines 28 to 50 in 17d8959

/// Calls parse_args_and_flags_from using argv (std::env::args_os)
///
/// This function is also responsible for treating and checking the command-line input,
/// such as calling [`canonicalize`](std::fs::canonicalize), checking if it the given files exists, etc.
pub fn parse_args() -> crate::Result<ParsedArgs> {
// From argv, but ignoring empty arguments
let args = env::args_os().skip(1).filter(|arg| !arg.is_empty()).collect();
let mut parsed_args = parse_args_from(args)?;
// If has a list of files, canonicalize them, reporting error if they do not exist
match &mut parsed_args.command {
Command::Compress { files, .. } | Command::Decompress { files, .. } => {
*files = canonicalize_files(files)?;
}
_ => {}
}
if parsed_args.flags.is_present("yes") && parsed_args.flags.is_present("no") {
todo!("conflicting flags, better error message.");
}
Ok(parsed_args)
}

@marcospb19 marcospb19 added the good first issue Good for newcomers label Oct 3, 2021
@marcospb19 marcospb19 added this to the 0.2.1 milestone Oct 7, 2021
@SpyrosRoum
Copy link
Contributor

Would it be better to use Error::Custom for this or create a new OofError variant like OofError::ConflictedFlag?
There already is OofError::FlagValueConflict but I don't think it's suitable for this

@marcospb19
Copy link
Member

@SpyrosRoum I think that Error::Custom would require less effort, and OofError is weird. However, we are planing on migrating to clap, and with clap we will be able to specify that those flags conflict, no need to manually add error variants.

The reasons why we chose to go with custom argparsing are now looking less important than the features we will get by using that crate, like shell completions and man pages (Also, editing the help message manually is painful).

That's a recent decision, so maybe I should create a issue for that.

Do you prefer closing this issue now or waiting for clap to solve it?

@SpyrosRoum
Copy link
Contributor

I can push the implementation I have with Error::Custom since it's just a couple of lines and could be included in 0.2.1 and then it can be replaced with clap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers high priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants