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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional typed base class for dynamic library API #4567

Closed
pekkaklarck opened this issue Dec 19, 2022 · 5 comments
Closed

Add optional typed base class for dynamic library API #4567

pekkaklarck opened this issue Dec 19, 2022 · 5 comments

Comments

@pekkaklarck
Copy link
Member

The main benefit would be automatic method name completion and type validation provided by IDEs.

Usage would be something like this:

from robot.api import DynamicLibrary

class Library(DynamicLibrary):
    ...

The base class should have all mandatory and optional dynamic API methods with appropriate type information and documentation. The mandatory methods should raise NotImplementedError and optional should return dummy information that's equivalent to not having these methods at all. We could also add a separate base class for the hybrid API although it only has get_keyword_names.

This is so easy to implement that I believe it could be done already in RF 6.1. The hardest part is deciding what the class name should be and should it be imported directly from robot.api or somewhere like robot.api.libraries, robot.api.interface or robot.api.base. A separate submodule could make sense if we add other similar base classes.

@pekkaklarck pekkaklarck added this to the v6.1 milestone Dec 19, 2022
@pekkaklarck pekkaklarck changed the title Add typed base class (interface) for dynamic library API Add optional typed base class for dynamic library API Jan 3, 2023
@pekkaklarck pekkaklarck added the good first issue Good for newcomers label Jan 3, 2023
@pekkaklarck
Copy link
Member Author

We can also add a base class also for the hybrid API. It will be pretty simple because it has only one method.

@pekkaklarck
Copy link
Member Author

DynamicLibrary and HybridLibrary base classes were added by the commit above and they can be seen here:
https://github.com/robotframework/robotframework/blob/master/src/robot/api/interfaces.py

This issue ought to be mostly done, but I leave it open for a little time to get possible review comments. We have also been discussing about this in a thread on the #devel channel on our Slack.

I'm somewhat worried there not being any tests for these base classes, but I'm not sure how to create meaningful tests. Just testing that classes exists and have correct methods feels like testing getters and setters.

@pekkaklarck
Copy link
Member Author

API docs for these base classes is visible here:
https://robot-framework.readthedocs.io/en/master/autodoc/robot.api.html#module-robot.api.interfaces

Sphinx doesn't see type aliases that have been used in signatures, and thus doesn't show List[Name] or Optional[ArgumentSpec] but instead has List[str] and Optional[List[Union[str, Tuple[str], Tuple[str, Any]]]]. Fixing that seems to require so much configuration that I don't consider it worth the effort.

@pekkaklarck
Copy link
Member Author

Some fixes and enhancements in 929055b.

Some testing with Robot, Libdoc and Mypy was done using the following code, and after the aforementioned fixes everything worked fine. Running this with Robot or Libdoc as part of out acceptance tests wouldn't bring much benefits going forward, because it's very unlikely our possible changes would break them. Testing with Mypy could be useful, but hooking it up to our test system is likely too much work compared to benefits. Easier to run it manually if typing is changed.

from typing import Any, List, Optional

from robot.api.interfaces import (Arguments, Documentation, DynamicLibrary, Name,
                                  NamedArgs, PositArgs, Source, Tags, Types)


class DynLib(DynamicLibrary):

    def get_keyword_names(self) -> List[Name]:
        return ['kw']

    def run_keyword(self, name: Name, args: PositArgs, named: NamedArgs) -> Any:
        return 42

    def get_keyword_arguments(self, name: Name) -> Optional[Arguments]:
        return ['a', ('b', 2)]

    def get_keyword_documentation(self, name: Name) -> Optional[Documentation]:
        return 'the doc'

    def get_keyword_tags(self, name: Name) -> Optional[Tags]:
        return ['t', 'a', 'g', 's']

    def get_keyword_types(self, name: Name) -> Optional[Types]:
        return {'a': int, 'b': (int, float)}

    def get_keyword_source(self, name: Name) -> Optional[Source]:
        return ':12'

@pekkaklarck
Copy link
Member Author

pekkaklarck commented Feb 7, 2023

I consider this issue done, but comments are still appreciated here or on Slack.

pekkaklarck added a commit that referenced this issue Feb 15, 2023
Apparently Mypy's stubgen doesn't like new `int|str` union syntax and
explicit `Union` needs to be used instead. Defining `Type` only once
outside `if/else` simplifies the code.
yanne pushed a commit that referenced this issue Mar 18, 2023
Apparently Mypy's stubgen doesn't like new `int|str` union syntax and
explicit `Union` needs to be used instead. Defining `Type` only once
outside `if/else` simplifies the code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants