Document type auto-conversion and test set and frozenset

[kdeldycke]

This is an ongoing exploration of usage of set and frozenset types in various places in the public API. It addresses the suspicion I had for a long time about inconsistent behavior, as detailed in #3036.

These tests for the moment only freeze the current behavior of Click. But reveals inconsistent or awkward behavior that are left to be discussed and decided upon for eventual bug fixes.

1. A set, frozenset or dict default is guessed as a STRING, not a container type (test_type_from_default_container, test_convert_type_from_container_default). This contradicts the documented rule of "If no type is provided, the type of the default value is used" Should we change that so we guess a container type instead of the current STRING?

2. An explicit type=set or type=frozenset is splitting the value provided to it (see test_explicit_container_type_splits_string). This looks like a bug we want to fix. My instinct is to split on commas, which overlaps an older discussion at: Allow nargs=-1 in options with a non-whitespace separator #2771 (comment) and my preference for it. Also dug out an old proposal at: Option value separators #422.

Also note that this also affects the envvars as demonstrated by my tests.

Closes #3036.

[davidism]

For 1, it should probably read "If no type is provided, the type of the default value is used if it's recognized, otherwise STRING is used."

Or it might have been trying to describe the current behavior, and been a little vague. "If no type is provided, the type of the default is used directly, rather than applying a Click param type. This can lead to unexpected behavior when passing the string value to the type constructor, such as set."

[davidism]

For 2, I do not want to add comma splitting to the core behavior. It's maybe appropriate for a type, but not auto detected, and I'm not sure it needs to be in core at all. It's also very easy for users to misunderstand, if they type a, b suddenly it parses differently. The old PR you found seems like a different thing, it's about allowing --key:value instead of --key=value (also something I don't want to add, I agree with the closure.)

[kdeldycke]

Thanks @davidism for the feedback.

For point 1, I agree it is a documentation issue. Even your answer is not really clear. So instead of trying to generalize the paragraph, I went with an exhaustive list of values and how they're interpreted by Click. See my updated documentation in the PR.

For point 2, yes, I was pointing to the --key:value/--key=value case as a way to demonstrate how we consistently rejected value splitting and over-interpretation over the years. Be it comma-splitting or key-value splitting. So let's forget about splitting for now, this is another thing that can be revisited independently in the future.

So all in all, I propose to make this PR a documentation fix that is also freezing the current Click behavior with unittests. That way it can reach 8.5.1.

[davidism]

I like the docs, and agree with adding tests to cover this behavior.

[Rowlando13]

This looks good. Are you ready for me to merge?

[kdeldycke]

This looks good. Are you ready for me to merge?

Yes! I don't like the result of these coverage, but at least it plugs documentation holes and lockdown the current behavior of Click. So this is a net benefits for the project. Let's keep the debate for another day! :)