-
Notifications
You must be signed in to change notification settings - Fork 139
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
Use variable typing in comments to support python 3.5 #81
Conversation
Not sure let me check my integration into my editor. |
I'm using |
sigkill_timeout: SomeSubstitutionsType = LaunchConfiguration('sigkill_timeout', default=5), | ||
prefix: Optional[SomeSubstitutionsType] = None, | ||
output: Optional[Text] = None, | ||
log_cmd: bool = False, |
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.
I thought these kinds worked in Python3.5?
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.
oh, maybe I went overkill. I'll check this afternoon.
6fbe7b7
to
3f71bfc
Compare
OK @wjwwood you were right and I had more changes than necessary before. It's been reduced to the minimal diff, which does include some code changes, if you could check them thanks (the changes are in individual commits). |
launch/launch/launch_introspector.py
Outdated
) | ||
return [msg] | ||
elif is_a(action, RegisterEventHandler): | ||
result = ["RegisterEventHandler('{}'):".format(action.event_handler)] | ||
result.extend(indent(format_event_handler(action.event_handler))) | ||
typed_action2 = cast(RegisterEventHandler, action) |
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.
mypy wasn't happy with reuse of the same variable name as above (even though they're in different branches)...
@@ -100,7 +103,7 @@ def format_event_handler(event_handler: EventHandler) -> List[Text]: | |||
"""Return a text representation of an event handler.""" | |||
if hasattr(event_handler, 'describe'): | |||
# TODO(wjwwood): consider supporting mode complex descriptions of branching | |||
description, entities = event_handler.describe() | |||
description, entities = event_handler.describe() # type: ignore |
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.
mypy was complaining about that EventHandler
doesn't have a describe
method: I think that ignoring it is the appropriate thing to do 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.
Yeah, the correct fix is to have an empty describe
method on the EventHandler
subclass. This is fine for now.
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.
Thanks for following through on this and for fixing up the files with unrelated mypy violations (I only added mypy to my editor about half way through the implementation).
@@ -55,7 +55,7 @@ class TimerAction(Action): | |||
def __init__( | |||
self, *, | |||
period: Union[float, SomeSubstitutionsType], | |||
actions: Iterable[LaunchDescriptionEntity], | |||
actions: Iterable[LaunchDescriptionEntity] |
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.
I don't think we need to do anything here (i.e. not blocking merge of this), but just so we're on the same page, this trailing comma was intentional. The idea being that it is allowed and ok style and it would reduce the diff if you needed to add more arguments in the future.
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.
Yeah, I know that we do it sometimes intentionally (thanks for the explanation though :) ) but it was causing syntax errors with coverage:
launch/actions/execute_process.py:37: in <module>
from .timer_action import TimerAction
E File "/home/dhood/ros2_ws/src/ros2/launch/launch/launch/actions/timer_action.py", line 59
E ) -> None:
E ^
E SyntaxError: invalid syntax
(presumably something unique to __init__
s)
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 looks like the python ticket, for the curious! (fixed in 3.6) https://bugs.python.org/issue9232
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.
Hmm, weird. :)
@@ -100,7 +103,7 @@ def format_event_handler(event_handler: EventHandler) -> List[Text]: | |||
"""Return a text representation of an event handler.""" | |||
if hasattr(event_handler, 'describe'): | |||
# TODO(wjwwood): consider supporting mode complex descriptions of branching | |||
description, entities = event_handler.describe() | |||
description, entities = event_handler.describe() # type: ignore |
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.
Yeah, the correct fix is to have an empty describe
method on the EventHandler
subclass. This is fine for now.
Fixes #78
This fixes the build on xenial.
@wjwwood there are a few lines that are too long (https://ci.ros2.org/job/ci_linux/4703/testReport/) but I haven't got mypy setup to check what wrapping will be supported. What invocation do you use? I get
Can't find package 'launch'
when callingpython3 -m mypy --package launch
...