StringListParamType interference when running cli multiple times in test suite

[gutzbenj]

Dear all, dear @lewoudar ,

this issue is meant to show some issue that we ran into within our test suite for wetterdienst at [1].

The test suite also tests our click based cli, which also leverages click_params StringListParamType at multiple points. We noticed that when running the cli with multiple pytest parametrized arguments, the first run/test may succeed but the second/third/... run would fail. This is because the cli is instantiated once and then uses the same instance of StringListParamType. There however at https://github.com/click-contrib/click_params/blob/daf87d9986937d2bc8df4def83fea18347a6cb80/click_params/base.py#L114-L129 we notice a attribute, that once the param type is used is changed and in the second/x run of that param type it will behave differently.

This should not effect the cli in running environment because there we only call the cli once, the cli is instantiated and afterwards it is shut down. However in the testing environment this may not be the case as multiple args could be tested against the same cli option/argument.

An example PR #23 (that atm fails as the current behaviour of StringListParamType is not adapted) shows this issue quite well.

We propose the following change for the above shown attribute:

def convert(self, value, param, ctx):
    if isinstance(value, list):
        return value

    if self._ignore_empty and value == "":
        return []
    value = self._strip_separator(value)
    errors, converted_list = self._convert_expression_to_list(value)
    if errors:
        self.fail(self._error_message.format(errors=errors), param, ctx)

    return converted_list

• isinstance would recognize if instead of a string a list would be passed
• a list would mean that the original value of type string has already been modified and the conversion would not be required anymore
• thus the value could be returned directly
• no double parsing done anymore

Please let us know what you think about this adaption!

Sincerely, Benjamin

/CC @amotl

[1] https://github.com/earthobservations/wetterdienst

[lewoudar]

Hi @gutzbenj, thank you for the feedback, I just remember that without the hack on lines 118 and 119, I have some weird errors. I will look at your proposition tomorrow or Tuesday when I will have more free time.

[lewoudar]

Ok, sorry for the late answer @gutzbenj , it sounds good to me. Would you mind update your PR with your fix? You will need to add a comment before checking value type Also if you want to do that, be aware that some tests will fail because I tested the private attribute _convert_called

[gutzbenj]

Sure! The PR was just updated and tests should be passing now.

[lewoudar]

Fixed by #23