-
Notifications
You must be signed in to change notification settings - Fork 13
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 runtime and static type checks #639
Conversation
Ping :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uff
@@ -221,9 +221,11 @@ | |||
"ros.distro": "iron", | |||
"search.useIgnoreFiles": false, | |||
"python.autoComplete.extraPaths": [ | |||
"/opt/openrobots/lib/python3.10/site-packages", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, but VS code always adds it and I dont want to make a special rule.
@@ -9,6 +10,7 @@ | |||
class AbstractBlackboardCapsule: | |||
"""Abstract class for blackboard capsules.""" | |||
|
|||
def __init__(self, node: Node, blackboard: Optional["BodyBlackboard"] = None): | |||
@nobeartype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is nobeartype needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has something to do with circular imports. The blackboard is imported for static type checking, but importing it for dynamic type checking leads to a circular dependency during runtime. Afaik bear type has no mechanism to handle this.
<test_depend>ament_mypy</test_depend> | ||
<test_depend>ament_cmake_mypy</test_depend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you depend on both? In other cases, you only use one of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe because this is a cmake python package and the other ones are setup.py. I don't remember.
Summary
Many of our typehints are wrong. This adds
beartype
based type checks during runtime and staticmypy
checks to prevent this.Proposed changes
This adds some risk, as it might lead to a less frequently visited code part with a minor type error crashing our codebase during a game. We could downgrade the errors to warnings, but then they might not be noticed/ignored. We could also deactivate them during real games, but we do not have a mechanic to check for this yet.
We need to test this on the real robot before merging it!
Checklist
colcon build