Codecov Report

Merging #3895 (1c5d9ee) into master (8e0c2a8) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3895      +/-   ##
==========================================
- Coverage   75.22%   75.16%   -0.05%     
==========================================
  Files         222      222              
  Lines       18789    18791       +2     
  Branches     3070     3071       +1     
==========================================
- Hits        14132    14123       -9     
- Misses       4128     4137       +9     
- Partials      529      531       +2     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e0c2a8...1c5d9ee. Read the comment docs.

The current functionality is there by design. The motivation is that we want conversion to be done if you have type like Union[int, str] and use argument like 42. I don't want that to be changed, but I don't like too much that currently if you have Union[int, float] and use it with ${42.9} that gets converted to an integer 42. Possibly we should always convert strings but with others with check is the type valid already. Anyway, this change needs a separate issue and cannot be done before RF 4.1.

The current functionality breaks optional strings Optional[str] completely.

If the purpose of this is to allow int/float/enum/None literals to override str then can we skip forced conversion when value is not a string

    def _no_conversion_needed(self, value):
        if isinstance(value, str):
            return False
        # None is not a valid type, convert to NoneType
        valid_instance_tuple = tuple(type(it) if it is None else it for it in self.args)
        return isinstance(value, valid_instance_tuple)

Or just always put str last in the conversion? I don't like this option as it's adding more complexity for the user.

    def _convert(self, value, explicit_type=True):
        if str in self.args:
            args = tuple(arg for arg in self.args if arg != str) + str
        else:
            args = self.args
        for typ in args:
            converter = TypeConverter.converter_for(typ)
            if not converter:
                return value
            try:
                return converter.convert('', value, explicit_type)
            except ValueError:
                pass
        raise ValueError

How does the current functionality break Optional[str]? This anyway needs an issue to discuss the design.

Optional[str] becomes Union[str, None] making it impossible to pass None from robot.

issue: #3897

I agree ${None} being converted to a string is not good if you use Optional[str], but I wouldn't say that totally breaks it. There's also a simple workaround that you omit Optional and just use arg=None. Anyway, let's discuss this more in #3897.

After thinking this more, I believe the approach this PR originally used is the best one. See my comment to issue #3897 for more details. I also submitted a separate issue about the problem with arg: Optional[str] = None (#3908).

Looks great. Docs related to Union conversion need to still be updated but I can do that myself. I'll also make sure we have tests covering the arg: Optional[str] = None case (#3908).

One of the tests was actually failing. I should've checked CI status more before merging, but I was anyway planning to enhance tests a bit so I'll fix the test at the same time.