Ability to register custom converters for keyword arguments

[DetachHead]

*** Test Cases ***
Example
    do thing    09/15/2021

maybe something like

@deco.keyword(converters={date: parse_date})
def do_thing(value: date):
    ...

def parse_date(value: str) -> date:
    ...

[pekkaklarck]

This would be a very convenient feature and having it already in RF 5.0 would be great. About the design:

1. New argument to the @keyword decorator like converters proposed above is probably the best way to enable this with individual keywords.

2. Converters need to be settable on library level as well to avoid the need to do that with each keyword. Using the @library decorator is probably the best approach. It could have converters as well and converters set on that level would be used by default.

3. Converters could be matched to arguments either based on argument name, position or type. The former two can nowadays be used when specifying argument types with @keyword, but I believe in this case working based on the type is the best approach. Otherwise specifying converters on the library level would be pretty much impossible. With individual keywords mapping converters based on position could possibly be convenient as it requires less code:

   @keyword(converters=[parse_date])
   def do_thing(value):
       ...

   Working based on position and by name would also allow using different converters even when arguments have same type:

   @keyword(converters={'first': parse_first_date, 'second': parse_second_date})
   def do_thing(first: date, second: date):
       ...

   Although there are potential use cases for mapping converters based on position and name, I believe it's best to not implement that at least in the beginning. Let's see if the use case gets actually important when this functionality works otherwise.

4. Libdoc should show documentation for these converters automatically. Possibly they could be included similarly as Enums and TypedDicts are at the moment (see Libdoc: Store information about enums and TypedDicts used as argument types in spec files #3607 and Libdoc: List enums and TypedDicts used as argument types in HTML automatically #3783, ping @Snooz82). The actual documentation shown for each converter could be simply taken from their docstrings.

5. Do we need to instruct custom converter creators about error reporting? For example, should we tell them to use ValueError and TypeError only? Or should we just catch all exceptions and report them? The latter is probably easier and kind of needed anyway as we don't want any random exception in conversion to crash the whole execution.

[DetachHead]

thanks for taking this idea on board!

but I believe in this case working based on the type is the best approach. Otherwise specifying converters on the library level would be pretty much impossible.

i agree. yesterday i actually attempted to create a decorator for keywords to add this functionality but got caught up in the rabbithole of runtime type-checking. for example:

@keyword(converters={list[date]: parse_date_range})
def do_thing(value: str, dates: list[date] | None) -> None:
    ...

how do you match the type of the dates parameter with the converter? you could create a custom version of issubclass that works on lists but then what about other types that use generics? something to think about...

[pekkaklarck]

It seems we can simply use equality check with generics:

>>> from typing import List, get_type_hints
>>> def f(arg: List[int]):
...    pass
... 
>>> typ = get_type_hints(f)['arg']
>>> typ == List[int]
True
>>> typ == List[str]
False
>>> typ == List
False

Probably should use issubclass as well to support derived types.

[pekkaklarck]

Are you @DetachHead interested to look at implementing this? I have many other tasks for RF 5.0 and getting help here would be great. I'm obviously happy to review PR and help also otherwise.

[pekkaklarck]

One more thing about the design: Library level defaults for converters should probably be read from a class or module level attribute like ROBOT_LIBRARY_CONVERTERS. It would be set by @library decorator but could also be set otherwise. That's the same approach we use e.g. with library scope.

[DetachHead]

@pekkaklarck i'm down to give it a go

[pekkaklarck]

Cool! Let me know if you need any help. Notice also that I'll shortly remote Python 2 support and that is likely to affect pretty much all code at least on some level.

[DetachHead]

@pekkaklarck sorry i haven't really had much time to work on this. i'm still interested to work on it but just don't know when i'll have the time

[pekkaklarck]

No worries. RF 5 work is now in good speed and Python 2 support has been removed. We'll concentrate on new syntax like TRY/EXCEPT first so there's plenty of time for you to get started. Keep us informed if you do!

[pekkaklarck]

I did some prototyping and now the basic functionality works on my machine. More precisely, the test in the original description (that I modified a bit) now passes with this library:

from datetime import date, datetime


def parse_date(value: str) -> date:
    return datetime.strptime(value, '%m/%d/%Y').date()


ROBOT_LIBRARY_CONVERTERS = {date: parse_date}


def do_thing(value: date):
    assert value == date(2021, 9, 15)

I need to still add error handling, support for @library(converters={...}) as well as tests before I can commit anything, but this certainly looks doable.

[pekkaklarck]

We still need to decide is it enough to be able to specify custom converters on the library level or should it be possible also with individual keywords. Specifying them only once is certainly more convenient than needing to do it with each keyword like

@keyword(converters={date: parse_date})
def do_thing(value: date):
    ...

@keyword(converters={date: parse_date})
def do_another_thing(value: date):
    ...

The above isn't even that much better than doing conversion manually:

def do_thing(value):
    value = parse_date(value)
    ...

def do_another_thing(value: date):
    value = parse_date(value)
    ...

Being able to specify custom converters would have two benefits, though:

1. Different keywords could have different converters. For example, a keyword expecting a date in Finnish format (%d.%m.%Y) could have a different converter than a keyword expecting a date in US format (%m/%d%Y):

   @keyword(converters={date: parse_us_date})
   def us_example(value: date):
       ...
   @keyword(converters={date: parse_fi_date})
   def fi_example(value: date):
       ...

2. On keyword level converters could be specified by name. Same type could thus have different converters like

   @keyword(converters={'us_date': parse_us_date, 'fi_date': parse_fi_date})
   def example(us_date: date, fi_date: date):
       ...

I'm starting to doubt the above benefits are big enough compared to how much more complicated implementation would get. Same benefits could also be got by having custom types like this:

class UsDate(date):
    pass

class FiDate(date):
    pass

def parse_us_date(value: str) -> date:
    return datetime.strptime(value, '%m/%d/%Y').date()

def parse_fi_date(value: str) -> date:
    return datetime.strptime(value, '%Y.%m.%d').date()


ROBOT_LIBRARY_CONVERTERS = {UsDate: parse_us_date, FiDate: parse_fi_date}


def us_example(value: UsDate):
    ...

def fi_example(value: FiDate):
    ...

def example(us_date: UsDate, fi_date: FiDate):
    ...

[pekkaklarck]

Another reason to limit converters only to the library level is that it would ease documenting them in Libdoc outputs like we do with Enums and TypedDicts (#3607). Part of this documentation is creating links to keywords that use these types, and a single type like date being used in different places with different converters would be odd.

[emanlove]

I like the workaround you show above creating a "pseudo" class of UsDate and FiDate that allows one to have a keyword based convertor while still maintaining the simplicity of the library level functionality. I was on the fence with the question of library versus keyword level implementation. I didn't have any real world examples to show support for either way but could see two keywords needing different convertors for the same type.

This sounds good to me.

[DetachHead]

Specifying them only once is certainly more convenient than needing to do it with each keyword like

@keyword(converters={date: parse_date})
def do_thing(value: date):
    ...

@keyword(converters={date: parse_date})
def do_another_thing(value: date):
    ...

The above isn't even that much better than doing conversion manually:

def do_thing(value):
    value = parse_date(value)
    ...

def do_another_thing(value: date):
    value = parse_date(value)
    ...

i believe the main benefit of this feature is type safety. in the second example, there's no type safety in the keyword's implementation because value is Any

[KotlinIsland]

One of the main benefits I see to this change is allowing dry-run to ensure valid inputs in tests.

@keyword(converters=dict(value=parse_date_phrase))
def do_another_thing(value: date):
    value
    ...

def parse_date_phrase(phrase: str) -> date: ...

some test
    Do Another Thing  two weeks from today

In this example, what I really want to happen is that the input string is validated statically.

[pekkaklarck]

@DetachHead, the reason you cannot have value: date in

def do_thing(value):
    value = parse_date(value)
    ...

is that then Robot would do automatic conversion to date which wouldn't work with input like 09/25/2021. Types that Robot doesn't convert to could be used as rype hints also when doing manual conversion.

@KotlinIsland Dry-run can work regardless are converters specified for each keyword or for the whole library. Validation is dynamic, not static, though.

[pekkaklarck]

One drawback of only supporting library level converters would be that with module based libs you couldn't use convenient

@library(converters={...})
class Lib:
    ...

and instead needed somewhat ugly

ROBOT_LIBRARY_CONVERTERS = {...}

With just a few keywords

@keyword(converters={...})
def kw(...):
    ...

could be nicer.

A good solution for this problem would be somehow making it possible to use the library decorator also with modules. It could then be used also with other things than converters. I've been thinking that we could support something like

LIBRARY = library(...)

to enable that. This would require a separate issue, though.

[DetachHead]

@DetachHead, the reason you cannot have value: date in

def do_thing(value):
    value = parse_date(value)
    ...

is that then Robot would do automatic conversion to date which wouldn't work with input like 09/25/2021. Types that Robot doesn't convert to could be used as rype hints also when doing manual conversion.

our use case is to override this functionality so that we can implement our own custom converters like in @KotlinIsland's example. sorry that i never got time to work on this though

[pekkaklarck]

@DetachHead Custom converters will certainly work as my early prototype demonstrated with date. Once I got the basic functionality with library level converters done and committed, hopefully today, I hope you can test how it works in your case. If it turns out keyword level converters would be convenient as well, we can take look at how complicated adding that support would be.

[emanlove]

For what is worth I think the drawback you mention, Pekka, is very minor. The important thing is that one can use custom converters. And it's not difficult nor requires a lot of code. It is just not the most visual pleasing syntax which, as you mention can be handle with a separate issue. Great job with this!

[DetachHead]

thanks so much for implementing this! a couple things:

• "Custom converters must accept exactly one positional argument" error when the second argument is optional. IMO it should allow functions that take more than one positional argument as long as they are optional
• how do you define converters for unions?

  # error: Custom converters must be specified using types, got UnionType datetime.date | None.
  ROBOT_LIBRARY_CONVERTERS = {date | None: convert_date}

  # error: Custom converters must be specified using types, got None None.
  ROBOT_LIBRARY_CONVERTERS = {date: convert_date, None: convert_date}

• it would be cool if you could define converters like this

  @converter(date)
  def convert_date(value: str) -> date:
      ...

[pekkaklarck]

Thanks a lot for testing the new functionality @DetachHead! Here are replies to your comments above:

1. I'm fine allowing more than one argument if they are optional. I just didn't personally see that functionality worth the little extra effort in checking argument counts and adding tests. I guess I could be convinced to add it and at least I'd be happy to merge a PR adding it.

2. You cannot create your own converters for Unions. In your case you ought to be able to so something like this:

   def convert_date(value: str | None) -> date:
       if value is None:
            ...
       ...
   
   ROBOT_LIBRARY_CONVERTERS = {date: convert_date}
   
   def keyword(arg: date):
       ...

   With the above implementation Robot would allow calling your keyword with a string or None (or date, it would go through without conversion) and your converter could then convert both types to date objects.

3. Registering a converter like @converter(date) would be convenient, but just

   from robot.api.deco import converter
   
   @converter(date):
   def convert_date(value):
       ...

   wouldn't work because that converter couldn't know where to register the converter. Something like

   from robot.api.deco import Converters
   
   converters = Converters()
   
   @converters.register(date):
   def convert_date(value):
       ...
   
   ROBOT_LIBRARY_CONVERTERS = converters

   could work and with class based libraries the last line could be even more convenient @library(converters=converters). The above could also be written like this:

   from robot.api.deco import Converters
   
   ROBOT_LIBRARY_CONVERTERS = Converters()
   converter = ROBOT_LIBRARY_CONVERTERS.register
   
   @converter(date):
   def convert_date(value):
       ...

   Implementing this ought to be pretty easy. Would others like this approach? I personally like the decorator syntax but still don't see too much benefits compared to just creating a dictionary directly.

[DetachHead]

wouldn't work because that converter couldn't know where to register the converter.

perhaps a way to declare converters globally? in our case we have a bunch of libraries and we want our converters to be accessible in all of them so it would be convenient if we could just define them in one spot. maybe the decorator could be the way to do that

[DetachHead]

here's another idea i had for globally defining converters, specifically for your own classes:

class RobotConvertable(ABC):
    @abstractmethod
    @classmethod
    def from_string(cls: type[Self], value: str) -> Self:
        ...


class Thing(RobotConvertable):
    @classmethod
    def from_string(cls: type[Self], value: str) -> Self:
        ...

@deco.keyword #robot checks each type to see if they extend RobotConvertable
def foo(value: Thing)
    ...

[pekkaklarck]

I thought about types themselves being converters so that if they contain a classmethod like from_string that would be called automatically. It's just not as generic solution than explicitly defining converters by type. It wouldn't, for example, allow adding custom converters for existing types.

Now that we have one way to define converters, I seriously doubt adding a new approach would bring much benefits. There would be quite a bit of extra work in implementation, testing and documentation, and for users having two ways could be confusing.

If you have a lot of libraries, explicitly defining converters is a little extra work but not that much. Converters themselves can be in a reusable module where they can be imported from.

[DetachHead]

found an issue where it doesn't like properties defined on the class:

class Foo:
    value1: str

    def __init__(self, value: str) -> None:
        self.value1 = value


ROBOT_LIBRARY_CONVERTERS = {Foo: Foo}


def bar(value: Foo) -> None:
    print(value)

robot throws the following error:

Type information given to non-existing argument 'value1'.

changing ROBOT_LIBRARY_CONVERTERS to {Foo: lambda value: Foo(value)} seems to fix it though

[pekkaklarck]

Thanks for the report @DetachHead. I was able to reproduce the problem and have already fixed it locally. Need to still add tests.

[pekkaklarck]

The problem was due to inspect.signature and typing.get_type_hints behaving differently when used with a class. The former takes signature from __init__ but the latter gets type hints from the class:

>>> class C:
...    a: int
...    def __init__(self, b: str):
...        pass
... 
>>> inspect.signature(C)
<Signature (b: str)>
>>> typing.get_type_hints(C)
{'a': <class 'int'>}

As the result type hints and signature didn't match. Can be Was fixed by explicitly getting type hints from __init__ with classes.

[DetachHead]

what's the expected behavior in cases like this?

ROBOT_LIBRARY_CONVERTERS = {date: parse_date}

def foo(value: date | str):
    ...

*** test cases ***
positive test
    foo  01/01/2020

test invalid dates
    foo  32/01/2020

currently in both tests it passes the value as a str (presumably because they were already strings to begin and since str is in the parameter's types it doesn't bother trying to convert it)

however in my case i want it to try and run the converter, and just fall back to a string if it fails.

[pekkaklarck]

That logic isn't related to custom converts but to how Unions are handled. If the given argument has any of the accepted types, it is passed as-is. That behavior has its problems but other alternatives had even more problems. One use case for custom converters actually is being able to control this fully yourself. See the following comment for a bit long explanation with links to relevant issues:
#4090 (comment)

[pekkaklarck]

This functionality is now documented in the User Guide. We only generate the User Guide with final releases, but you the reStructuredText source files are rendered pretty well on GitHub. This particular documentation can be found here:
https://github.com/robotframework/robotframework/blob/master/doc/userguide/src/ExtendingRobotFramework/CreatingTestLibraries.rst#custom-argument-converters

It would be great if people interested in this functionality would check the docs and comment if there's something to be enhanced or fixed (incl. typos).

[pekkaklarck]

This issue ought to now be done. How custom types will be shown/stored in Libdoc outputs is still likely to change, but that is covered by #4160. Documentation can still be enhanced and I leave this issue open for few days to wait for comments related to that.

[pekkaklarck]

No comments so far and I close this issue. That doesn't mean comments weren't appreciated in the future!