CLI: Fail command if --config file contains unknown key#5939
Conversation
|
@ltalirz @ConradJohnston proof of principle implementation that I think should work. As noted in the issue, due to the design of |
|
Thanks @sphuber ! I personally don't see a great downside to vendoring this code in AiiDA; I'd suggest to add a few tests here & merge the PR (removing the dependency). In the meanwhile, open a PR to click-config-file; if they're happy to accept it, we can remove the code from AiiDA again. How does that sound? |
sphuber
force-pushed
the
fix/5929/cli-config-option-detect-unknown-keys
branch
from
676415d to
3c90197
Compare
Up till now, the `--config` option would accept configuration files with keys that did not correspond to existing options on the command. This would lead to incorrect options to be swallowed silently, which can be surprising to users if they accidentally misspelled an option or used one that didn't exist. Here the callback is updated to check the specified keys in the config and cross-reference them to the params that are defined on the command. If any unknown keys are specified a `BadParameter` exception is raised which displays which keys are not supported. The implementation was originally provided by the `click-config-file` dependency, however, in order to customize it with the desired behavior we had to essentially vendor the entire code, since there was no available hook for the customizations. The code is taken from https://github.com/phha/click_config_file/blob/7b93a20b4c79458987fac116418859f30a16d82a which corresponds to `v0.6.0`. Besides the main changes, the default provider is changed to `yaml_config_file_provider`, the docstrings are reformated and type hints are added.
sphuber
force-pushed
the
fix/5929/cli-config-option-detect-unknown-keys
branch
from
3c90197 to
0b784e6
Compare
|
Opened a PR on |
There was a problem hiding this comment.
Awesome work thus far.
So I do the following test:
I take an existing code, and do verdi code export to generate a yml.
Then I try to generate a new code from this yml:
verdi code setup --config test.yml.
It chokes on two keys:
Error: Invalid value for '--config': Invalid configuration file, the following keys are not supported: {'default_calc_job_plugin', 'filepath_executable'}
I would have expected a reversible process, but something isn't working as expected.
It seems to be that the options in the CLI do not match the keys.
filepath_executable is given when click expects --remote-abs-path
default_calc_job_plugin should be --input-plugin
All of this said, I think this should be a separate issue and is not a problem with this PR.
It seems that the hardcoded dict _get_cli_options()is out of sync with the CLI.
|
|
||
| if unknown_params: | ||
| raise click.BadParameter( | ||
| f'Invalid configuration file, the following keys are not supported: {unknown_params}', ctx, param |
There was a problem hiding this comment.
A nitpick:
Is BadParameter the best error class to use here?
I feel like the parameter to the option --config is the path or URL.
That said, the other click error classes are no better.
There was a problem hiding this comment.
I am open to suggestions for better exceptions but I couldn't find one. Note that for the exception to be properly handled, it needs to be a click exception. I think it makes sense to say there is a problem with the --config option, because its value (the content of the file) is incorrect. Since the problem is likely that it contains unsupported keys, we couldn't have it be coupled to any other option of the command, as they don't exist.
This is expected though. The |
Ok, I understand now, thank you! |
3 participants
Fixes #5929
Up till now, the
--configoption would accept configuration files with keys that did not correspond to existing options on the command. This would lead to incorrect options to be swallowed silently, which can be surprising to users if they accidentally misspelled an option or used one that didn't exist.Here the callback is updated to check the specified keys in the config and cross-reference them to the params that are defined on the command. If any unknown keys are specified a
BadParameterexception is raised which displays which keys are not supported.N.B.: The use of the parameter name to filter out the config option itself from valid options is a bit fragile, but I couldn't find a better way for now since the type is not stored and there is no other uniquely defining attribute or type that I could distinguish it with.This is solved, the name of the config option is actually passed as an argument in the callback so we can use that.