Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

(馃巵) Support typing.overload in type converters #4220

Open
KotlinIsland opened this issue Feb 7, 2022 · 13 comments 路 May be fixed by #4642
Open

(馃巵) Support typing.overload in type converters #4220

KotlinIsland opened this issue Feb 7, 2022 · 13 comments 路 May be fixed by #4642

Comments

@KotlinIsland
Copy link
Contributor

KotlinIsland commented Feb 7, 2022

@overload
def a(*, a: int) -> None: ...

@overload
def a(*, b: int) -> None: ...

def a(*, a: int=0, b: int=0) -> None: ...
Testcase
    A  a=1  b=2

When this is run no error is shown, I think it would be cool if robot could look at overloads and show an error here.

@pekkaklarck
Copy link
Member

I assume you mean typing.overload. As you can read from its docs, overloading information isn't available at runtime and thus not accessible for Robot. Robot only sees the last definition. Do you have ideas how that problem could be handled? What kind of error should actually occur in this case?

@DetachHead
Copy link
Contributor

i ran into the same problem when trying to add overloads to the playwright api. i wrote a wrapper for @overload that makes the information accessible at runtime. microsoft/playwright-python#851

perhaps can do something similar here?

@pekkaklarck
Copy link
Member

Do you mean Robot should have its own @overloads? Could you clarify what benefits it would bring? Related to that, what should happen with the example in the original description?

@KotlinIsland
Copy link
Contributor Author

I think this is out of scope for robotframework. I'll raise it on rflint and see what they think.

@KotlinIsland
Copy link
Contributor Author

@pekkaklarck Overloaded functions can now be introspected at runtime with typing.get_overloads

@KotlinIsland KotlinIsland reopened this May 17, 2022
@KotlinIsland KotlinIsland changed the title Support typing.overload (.presSupport typing.overload Feb 5, 2023
@KotlinIsland KotlinIsland changed the title (.presSupport typing.overload (馃巵) Support typing.overload Feb 5, 2023
@KotlinIsland KotlinIsland linked a pull request Feb 5, 2023 that will close this issue
@KotlinIsland KotlinIsland changed the title (馃巵) Support typing.overload (馃巵) Support typing.overload in type converters Feb 5, 2023
@pekkaklarck
Copy link
Member

Could you clarify what benefits this enhancement would have? What new functionality would it bring over using, for example, unions?

@pekkaklarck
Copy link
Member

One big challenge implementing this would be deciding how overloaded methods are shown in Libdoc outputs. Just showing arguments for the final, non-overload implementation would mean that users couldn't see any info about the actual accepted types. You'd thus needed to document them separately in the docstring making this a really unattractive option. I would say that automatic type documentation is a required feature.

A problem with adding the info about overloads to Libdoc is that it may require changes to Libdoc spec files. These spec files are part of the public API so changes should preferably be backwards compatible.

@KotlinIsland
Copy link
Contributor Author

Could you clarify what benefits this enhancement would have? What new functionality would it bring over using, for example, unions?

I don't really know what you mean, overloads provide distinct signatures, what would unions have to do with this?

@pekkaklarck
Copy link
Member

pekkaklarck commented Feb 13, 2023

With overloads you can do this:

@overload
def should_be_positive(number: int):
    ...
@overload
def should_be_positive(number: float):
    ...
def should_be_positive(number): 
    if number <= 0:
        raise AssertionError(f'{number} is not positive')

If Robot supported overloads, these would work:

*** Test Cases ***
Example
    Should be positive    1
    Should be positive    1.1

That would be handy, but you can already now use unions like this:

def should_be_positive(number: int|float): 
    if number <= 0:
        raise AssertionError(f'{number} is not positive')

In my opinion the union variant is a lot simpler and easier to understand than the variant using overloads. I know that if there are more arguments and/or return values, overloads can help to make typing more precise. I just don't know about any use case with Robot where such functionality would actually be useful. Do you have some concrete use case where overloads would help?

@KotlinIsland
Copy link
Contributor Author

You are correct that your example is an invalid use case for overloads, see the OP for a semi realistic use case.

@DetachHead
Copy link
Contributor

DetachHead commented Feb 14, 2023

overloads can do much more than that

@overload
def should_be_positive(number: int):
    ...
@overload
def should_be_positive(number: float, round_to: int | None = ...):
    ...
def should_be_positive(number, round_to = None):
    if round_to is not None:
        number = round(number, round_to)
    if number <= 0:
        raise AssertionError(f'{number} is not positive')
*** Test Cases ***
Example
    Should be positive    1            # no error
    Should be positive    1.1  True    # no error
    Should be positive    1    True    # error

@pekkaklarck
Copy link
Member

I already wrote that

I know that if there are more arguments and/or return values, overloads can help to make typing more precise

and your example just gives one such example. My question was, do you have concrete use cases where that matters?

Anyway, I don't have anything against adding support for overloads. The main reason I'd like to know about concrete use cases is to figure out how high priority this issue should have. If it's generally useful, I can spend time for this still before RF 6.1, but it is just nice-to-have I need to concentrate on other issues with higher priority.

The big open issue related to overloads is designing how they should be represented in Libdoc's spec files and shown in Libdoc's HTML outputs. Without good Libdoc support this feature is rather useless and adding it doesn't make any sense. I hope you can study that yourself and propose a solution here. Unless you can convince me this is a high priority enhancement, I won't have time to help you too much, but I'll certainly review and comment your proposal.

@KotlinIsland
Copy link
Contributor Author

KotlinIsland commented Feb 17, 2023

Okay, here are some examples pulled from a private codebase:

@overload
def open_browser(
    url: str,
    browser_name: Literal["firefox"],
    *,
    firefox_user_prefs: dict[str, float | str | bool] | None = ...,
): ...

@overload
def open_browser(
    url: str,
    browser_name: Literal["chromium"] | None = ...,
): ...

@keyword
def open_browser(
    url,
    browser_name=None,
    *,
    firefox_user_prefs=None,
):
@overload
def create_new_hire(
    person: Employee, *, position_id: int, additional_position_ids: list[int] = ...
):
    ...


@overload
def create_new_hire(person: Employee, *, position_id: int, is_transfer: bool = ...):
    ...


@keyword
def create_new_hire(
    person,
    *,
    position_id,
    additional_position_ids: list[int] | None = None,
    is_transfer=False,
):

In this example it's not valid for a transfer to have additional positions, so the overloads only allow one or the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants