Validate parsed config against CLI options

[atugushev]

Refs: #1909.

Config key validation

$ cat .pip-tools.toml
[tool.pip-tools]
annotation = false

$ pip-compile
Error: No such config key: annotation. (Possible options: annotate, annotation_style)

Config value validation

$ cat .pip-tools.toml
[tool.pip-tools]
dry-run = ["foo", "bar"]

$ pip-compile
Error: Invalid value for config key 'dry-run': ['foo', 'bar']

Contributor checklist

• Provided the tests for the changes.
• Assure PR title is short, clear, and good to be included in the user-oriented changelog

Maintainer checklist

• Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
• Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

[atugushev]

FYI: I have removed the options for pip-sync (which led to test failures) because we only test the pip-compile CLI here. We could add a separate test for pip-sync with its own options, but it seems redundant to me.

[chrysle]

Could we add the below condition? It would convert all negating boolean flags to their valid equivalents, e.g. no-annotate = true -> annotate = false, but error out in case these equivalents or the given values do not exist.

Of course the suggestions for non-valid flags would have to be adapted...

[chrysle]

Suggested change

	for key, value in config.items():
	# Validate unknown keys
	if key not in cli_params:
	possibilities = difflib.get_close_matches(key, cli_params.keys())
	raise click.NoSuchOption(
	option_name=key,
	message=f"No such config key {key!r}.",
	possibilities=possibilities,
	ctx=click_context,
	)
	# Validate invalid values
	param = cli_params[key]
	try:
	param.type.convert(value=value, param=param, ctx=click_context)
	except Exception as e:
	raise click.BadOptionUsage(
	option_name=key,
	message=f"Invalid value for config key {key!r}: {value!r}.",
	ctx=click_context,
	) from e
	swap_keys = False
	new_items = {}
	old_items = {}
	for key, value in config.items():
	# Validate unknown keys
	if key not in cli_params:
	# Replace boolean flag contexts like ``no_annotate`` with their equivalents
	if key.startswith("no_"):
	old_items[key] = value
	key = key[3:]
	value = not value
	swap_keys = True
	new_items[key] = value
	else:
	possibilities = difflib.get_close_matches(key, cli_params.keys())
	raise click.NoSuchOption(
	option_name=key,
	message=f"No such config key {key!r}.",
	possibilities=possibilities,
	ctx=click_context,
	)
	# Validate invalid values
	param = cli_params[key]
	try:
	param.type.convert(value=value, param=param, ctx=click_context)
	except Exception as e:
	raise click.BadOptionUsage(
	option_name=key,
	message=f"Invalid value for config key {key!r}: {value!r}.",
	ctx=click_context,
	) from e
	if swap_keys:
	for key, value in old_items.items():
	del config[key]
	for key, value in new_items.items():
	config[key] = value

[atugushev]

That makes sense indeed. I would prefer to implement this in the parse_config_file() function though. The _validate_config() function does not intend a config mutation.

[chrysle]

In that function, you might be able to implement it like this:

    swap_items = False
    new_items = {}
    old_items = {}

    for key, value in piptools_config.items(): 
        # Replace boolean flag contexts like ``no_annotate`` with their equivalents
        if key.startswith("no_"):
            swap_items = True
            old_items[key] = value
            if isinstance(value, bool):
                value = not value
            new_items[key[3:]] = value

    if swap_items:
        for key, value in old_items.items():
            del piptools_config[key]
        for key, value in new_items.items():
            piptools_config[key] = value


    return piptools_config

[atugushev]

Would you like to address this in a following up PR?

[chrysle]

Sure!

[atugushev]

Great, thanks 👍🏻

[atugushev]

As a reminder to myself: I think we can refactor parse_config_file to parse options using the technique from the following snippet.

Snippet

(Pdb++) opts = {
    opt: option 
    for option in ctx.command.params
    for opt in itertools.chain(option.opts, option.secondary_opts)
}

(Pdb++) pp opts
{'--all-extras': <Option all_extras>,
 '--allow-unsafe': <Option allow_unsafe>,
 '--annotate': <Option annotate>,
 '--no-annotate': <Option annotate >,
 ...
}
 
(Pdb++) opts['--pip-args'].name
'pip_args_str'

(Pdb++) opts['--annotate'].name
'annotate'

(Pdb++) opts['--no-annotate'].name
'annotate'

(Pdb++) opts['--extra'].name
'extras'

[chrysle]

Looks good! I guess this would enable us to rationalise the function get_click_dest_for_option() away. Would you like me to address that too, while I'm at it?

[atugushev]

Sure, feel free to!

[AndydeCleyre]

This looks good to me, thanks!

Can you include the exception types to expect, click.BadOptionUsage and click.NoSuchOption, in the _validate_config docstring?

[atugushev]

Can you include the exception types to expect, click.BadOptionUsage and click.NoSuchOption, in the _validate_config docstring?

@AndydeCleyre thanks! Addressed in 278d8df.

[atugushev]

Thanks @AndydeCleyre and @chrysle for the review! 🙏🏻