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.
I thought these kinds worked in Python3.5?
There was a problem hiding this comment.
oh, maybe I went overkill. I'll check this afternoon.
dhood
force-pushed
the
commented_variable_typing
branch
from
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). |
dhood
added
in review
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.
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.
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.
Yeah, the correct fix is to have an empty describe method on the EventHandler subclass. This is fine for now.
|
xenial |
| @@ -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.
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.
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.
this looks like the python ticket, for the curious! (fixed in 3.6) https://bugs.python.org/issue9232
| @@ -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.
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...