Improve error for args_conflicts_with_subcommands
#3939
Conversation
Before this patch when running into errors due to `args_conflicts_with_subcommands`, you'd get an output looking like this: ``` error: The subcommand 'auth' wasn't recognized Did you mean 'auth'? ``` This is highly confusing and leaves the user without any information on what actually went wrong. This patch addresses this problem by introducing a new error message dedicated for this scenario which will inform the user that the specific combination of argument and subcommand used is not valid.
| @@ -297,6 +297,22 @@ impl Error { | |||
| ]) | |||
| } | |||
|
|
|||
| pub(crate) fn argument_subcommand_conflict( | |||
There was a problem hiding this comment.
I'm hijacking the ArgumentConflict error here, it would probably make sense to introduce a new error kind specifically for this, but I didn't want to spend hours on error boilerplate with a PR without first getting some feedback on the general approach.
| // Already met any valid arg(then we shouldn't expect subcommands after it). | ||
| let mut valid_arg_found = false; | ||
| // Already met this valid arg(then we shouldn't expect subcommands after it). | ||
| let mut valid_arg_found = None; |
There was a problem hiding this comment.
Turning this into an Option here gives us the nice option to tell the user which flag caused the subcommand to fail. Ideally they can then remove the listed subcommand and if another error pops up due to extra flags, they can repeat the process until all flags are removed.
| // Conflicting arg and subcommand due to `args_conflicts_with_subcommands`. | ||
| let subcmd_match = self.possible_subcommand(arg_os.to_value(), &None); | ||
| if let Some((arg, subcmd)) = valid_arg_found.zip(subcmd_match) { | ||
| return ClapError::argument_subcommand_conflict( | ||
| self.cmd, | ||
| arg, | ||
| subcmd.into(), | ||
| Usage::new(self.cmd).create_usage_with_title(&[]), | ||
| ); | ||
| } |
There was a problem hiding this comment.
I was hoping to handle this before the error function, but it seems like it's not possible to error out immediately on the subcommand because it might still be a valid argument.
I'm not entirely sure if this is a bullet-proof way to catch this error, but it's the best solution I could find. So let me know if there's a better place to handle this.
There was a problem hiding this comment.
This is one of my biggest concerns with trying to improve the error in this situation. With this not being a area of priority at the moment, this means you'll need to help carry a lot of the load in analyzing and verifying this doesn't regress other parts of the user experience.
There was a problem hiding this comment.
That much should be covered both by tests and by the conditions checked before this error is emitted though, right?
Do you think there's additional changes necessary to ensure this patch does not have any unwanted side effects?
There was a problem hiding this comment.
Its also thinking of all of the different user cases and considering the error message under each one
for todo --flag sub1 we currently give
error: The argument '--flag' cannot be used with subcommand 'sub1'
What other intentions could the user have for todo --flag sub1?
What other combinations of flags might hit this error but be unrelated?
There was a problem hiding this comment.
What other combinations of flags might hit this error but be unrelated?
I mean hopefully none, that was the goal at least. I couldn't find any.
What other intentions could the user have for todo --flag sub1?
Providing additional information on what the user might want to do is just an improvement on the error message though, right? I wouldn't say there's a good recommendation that can be made here other than removing the flag, which I'd say this error does reasonably well.
Ultimately though even if this error can be improved (maybe even in the future), I think if the question is if we can provide a good enough error here, we should also take into account that the status quo is far worse. So much so that it makes the option unusable (imo).
| // Conflicting arg and subcommand due to `args_conflicts_with_subcommands`. | ||
| let subcmd_match = self.possible_subcommand(arg_os.to_value(), &None); | ||
| if let Some((arg, subcmd)) = valid_arg_found.zip(subcmd_match) { | ||
| return ClapError::argument_subcommand_conflict( | ||
| self.cmd, | ||
| arg, | ||
| subcmd.into(), | ||
| Usage::new(self.cmd).create_usage_with_title(&[]), | ||
| ); | ||
| } |
There was a problem hiding this comment.
This is one of my biggest concerns with trying to improve the error in this situation. With this not being a area of priority at the moment, this means you'll need to help carry a lot of the load in analyzing and verifying this doesn't regress other parts of the user experience.
|
As an FYI, #4567 improved the error message somewhat but not enough to supersede this PR |
Before this patch when running into errors due to
args_conflicts_with_subcommands, you'd get an output looking likethis:
This is highly confusing and leaves the user without any information on
what actually went wrong.
This patch addresses this problem by introducing a new error message
dedicated for this scenario which will inform the user that the specific
combination of argument and subcommand used is not valid.