Value conversion for CategoricalParameter and TaskParameter

[myrazma]

I experienced unexpected behaviour in the validation when using a string as input type for active_values in the TaskParamter instead of a list.

I created the TaskParameter with a single-char string for active_values, i.e. active_value="A" for testing purposes. The Campaign object could be created without a problem. Unexpectedly, I got an error when creating the Campaign object in case the string for active_values was longer than 1, for example active_value="C12":

As @Scienfitz already pointed out, it is related to the input type, i.e. list vs string, and that the string is interpreted as a list in the validation. Therefore "C" is not in ["C12", ..] fails whereas "A" is not in ["A", ..] works in case of a single-char string. To provide robustness or a clearer error message in that case, the validation could check whether active_values is a list or string and act accordingly.

When I input active_values as a list, i.e. active_value=["C12"], the validation and creation of the Campaign object works without problems.

[Scienfitz]

As discussed, the function works as intended with correct inputs, but can have improper validation in the demonstrated example, accepting improper input causing a downstream error.

This is due to the automatic structuring of a string as list. If its possible to avoid this automatic conversion this loophole should be fixable.

[AdrianSosic]

There is an ugly and a nice solution to this. The core of the problem is that we convert the input of values/active_values to tuple and once this is done, there is no way you can tell whether the original input was a string or any other iterable. That is:

• `tuple("ABC") --> ("A", "B", "C") (which we don't want to happen)
• `tuple(["A", "B", "C"]) --> ("A", "B", "C") (which we do want to happen)

So the only way to catch that first case is to do a check before the conversion happens. It attrs speech, that would mean before calling the converter or as part of the converter.

So the "ugly" solution would be to write a converter utility that does the check and use it instead:

def nonstring2tuple(x: Iterable) -> Tuple:
    if isinstance(x, str):
        raise ValueError("Input must not be a string.")
    return tuple(x)

This would only require to replace converter=tuple with converter=nonstring2tuple in placed where we want that check to happen. However, the downside (which I think is a critical one) is that we can hardly raise specific error messages that way.

The "nice" solution would be to use a three-argument converter that also gets the respective context like the validators (i.e., not only the to-be-converted value but also self and field), such that we can build a context-specific error message inside the converter. However, this is not (yet) supported by attrs and has been raised in tickets since long time ago. Luckily, there's now an open PR since 4 days that seems about to be merged (perfect timing 😄). So I suggest we wait until the next attrs version is released and solve the problem then 👍🏼

[Scienfitz]

@AdrianSosic I don't consider the second solution ugly and would support that as an immediate fix. In fact we could debate whether we can also explicitly allow a single str and and return (x,) or so if isinstance(x, str) instead of the raise. But even if not it would solve stuff immediately

[AdrianSosic]

I've heard the new attrs version is around the corner (should've already happened before PyCon), meaning that the new Converter wrapper will be available soon. Thus, I directly implemented the proper fix in #259