Add pyroject.toml with mypy section and type annotations #4043
Conversation
|
There are lot of APIs where adding type information helps, but I don't think adding them everywhere and making MyPy happy is worth the effort. My biggest problem is the added noise in cases like these:
There's also the problem that RF 5.0 will still support Python 3.6 and we cannot use more modern typing features. For example, we couldn't use This PR also has a problem that it was started before we removed Python 2 support and there are thus a lot of conflicts. Before spending more time for this, it would be a good idea to discuss this more either here or on Slack. As I already wrote, I don't think being MyPy compatible is worth the effort, but if you can list good concrete benefits we'd get I may be convinced otherwise. Adding type information to public APIs is definitely a good idea, though, but should probably be done so that there's an issue/PR for each API. One API that would especially benefit from type information is visitor. Changing current class Visitor:
...
def start_test(self, test):
...to from robot.model import TestCase
class Visitor:
...
def start_test(self, test: TestCase):
...would mean that users of this API would get support for auto-completion. That's a concrete benefit and there's no extra noise either. |
Type comments aren't magic, they are part of PEP 484
I completely agree (python/mypy#9413), but is currently a requirement.
The redundant annotation when there's a default argument is questionable, but I think it's better to be explicit.
Not sure what you mean here, all these features are standard typing constructs:
It's valid to write any type annotation as a string literal, so no imports are needed and new syntax can be used: a: "dict[str | None, list[int] | list[str]]"This does have the disadvantage that Also
Don't you think that by adding static type information to the internal api it would make development / refactoring a much smoother process? |
|
i think typing even the internal API would be highly beneficial especially for new contributors. in my experience, projects with types are so much easier to jump right in and start making changes with little to no experience with the codebase, because it allows your IDE to help out a lot when looking for a property you need or when you're trying to figure out how to call a function. another huge benefit is that the type checker informs me of other parts of the code i may have broken |
71b453d to
d758f54
Compare
No I don't. For me most changes this PR just make the code harder to understand because there's so much extra noise. I don't like how the code looks and I'd even less like to write code like this. If I'd like static typing, I'd use Java or C#. I see benefits from type hints in selected APIs, especially in public APIs, but I really see more problems than benefits with full MyPy support. |
This is a valid point, I'm not the only one working with this code base. I'm not sure is this true with all new contributors, though. Typing in Python, especially if you want full MyPy compatibility, is somewhat advanced topic and lot of our contributors are novice programmers. I'm afraid this could be a barrier of entry for people who aren't already familiar with typing. |
|
Comming into this project, it feels very overwhelming, with multiple classes with the same name and functions with very ambiguous types. Even trying to figure out the types for this change has been extremely confusing. |
|
I'm sure we can add types to various places to make the code easier to understand and to ease navigation around. Adding more documentation and creating some kind of an architecture diagram could help as well. It's the full MyPy compatibility that bothers me. A very good example of noise it adds without benefits is the robot.api.deco module. The original code is pretty trivial (assuming you know how decorators work), but the added def keyword(name=None, tags=(), types=()):to def keyword(name: Union[Fn, str, None] = None, tags: Union[List[str], Tuple[None, ...]] = (), types: Union[Dict[str, type], List[type], Tuple[None, ...], None] =()) -> Union[Callable[[Fn], Fn], Fn]:make it a lot more complicated. As far as I can see, none of the changes in that file actually has any concrete benefits. |
|
Mypy is very flexible around what needs to have annotations / what is checked. Everything is entirely optional and confurable down to the individual module. Tbh in my projects that use robot framework the Maybe there is a way to simplify this type annotation. |
Could you clarify this? Does the variant in this PR fix the problem? |
|
When a decorator isn't typed the signature of the original function becomes dynamic(basically completely uncheckd), there is a mypy rule relating to this https://mypy.readthedocs.io/en/stable/config_file.html#confval-disallow_any_decorated. @untyped_decorator
def foo(a: int) -> str: ...
reveal_type(foo) # Any
So when I do have an idea about how to drastically simplify this implementation but will need to get to it tomorrow. |
@overload
def keyword(name: Fn) -> Fn: ...
@overload
def keyword(name: str = None, tags: List[str] = ..., types: _Types = ...) -> Callable[[Fn], Fn]: ...
@not_keyword
def keyword(name: object = None, tags: object = (), types: object = ()) -> object:Simplified the defs to this |
…mypy src/robot/api` runs successfully
|
@pekkaklarck If you think type annotations look gross they could be implemented as separate |
Now that development has started on rf 5 which is dropping support for python 2.7-3.5 adding type annotations and mypy integration is a good idea (imo).
Annotations have been added such that
mypy src/robot/apiruns successfully.