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

Displaying the help should take precedence over checking env vars & other possible checks #925

Closed
w3lld0ne opened this issue Sep 23, 2023 · 2 comments · Fixed by #926
Closed
Labels

Comments

@w3lld0ne
Copy link

This is an issue which was only slightly mentioned in #903, so I will describe this specific case here in more details.

Basic example:

auto CliApp = new CLI::App("myapp");
auto sub1 = CliApp->add_subcommand("sub1");
std::filesystem::path someFile = "";
sub1->add_option("-f,--file", someFile)->envname("SOME_FILE")->required()->expected(1)->check(CLI::ExistingFile);

try
{
	CliApp->parse(argsCount, args);
}
catch (const CLI::ParseError& e)
{
	return CliApp->exit(e);
}

Make sure %SOME_FILE% env var is defined as invalid/non-existing file path.
Then call the app like that: myapp.exe sub1 -h:
-Normally it should only display the help about sub1 expectations & requirements and exit, as that's a completely separate control flow path for the app, just like displaying the app version with -v.
-Actually it fails CLI::ExistingFile validator check, suggesting to Run with --help for more information.. But it's impossible to call for help.

@phlptp phlptp added the bug label Sep 23, 2023
@phlptp
Copy link
Collaborator

phlptp commented Sep 23, 2023

I was able to replicate the issue. Working on a fix. Thanks

@phlptp
Copy link
Collaborator

phlptp commented Sep 23, 2023

The issue stems from the fact that validation errors are given higher precedence then the help "error" trigger. This is a deliberate choice. In most cases if you really want help you can remove other arguments. In the case of environmental values, that fail validation there is no way to not trigger it. This is probably not a good situation. The fix I would propose is to make the environmental variables do a pre-validation, like the validate_positionals() modifier. Except in this case it is not an optional modifier. It would be like not having the environmental variable defined to begin with if it doesn't pass validation. This gets around the issue described in the bug report. There might be other possible solutions if CLI11 were to ever work through all the details to make it work with no exceptions, but for now I think prevalidating the results for environmental variables is probably the best and simplest approach. I will make a PR shortly and have @henryiii check to make sure that is approach we want to take.

@phlptp phlptp mentioned this issue Sep 23, 2023
phlptp added a commit that referenced this issue Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants