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

Use variable typing in comments to support python 3.5 #81

Merged
merged 7 commits into from
Jun 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions launch/launch/actions/execute_process.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import signal
import threading
import traceback
from typing import Any
from typing import Any # noqa: F401
from typing import cast
from typing import Dict
from typing import Iterable
Expand Down Expand Up @@ -52,7 +52,7 @@
from ..launch_description import LaunchDescription
from ..some_actions_type import SomeActionsType
from ..some_substitutions_type import SomeSubstitutionsType
from ..substitution import Substitution
from ..substitution import Substitution # noqa: F401
from ..substitutions import LaunchConfiguration
from ..substitutions import PythonExpression
from ..utilities import create_future
Expand All @@ -79,7 +79,7 @@ def __init__(
sigkill_timeout: SomeSubstitutionsType = LaunchConfiguration('sigkill_timeout', default=5),
prefix: Optional[SomeSubstitutionsType] = None,
output: Optional[Text] = None,
log_cmd: bool = False,
log_cmd: bool = False
) -> None:
"""
Construct an ExecuteProcess action.
Expand Down Expand Up @@ -156,7 +156,7 @@ def __init__(
super().__init__()
self.__cmd = [normalize_to_list_of_substitutions(x) for x in cmd]
self.__cwd = cwd if cwd is None else normalize_to_list_of_substitutions(cwd)
self.__env: Optional[Dict[List[Substitution], List[Substitution]]] = None
self.__env = None # type: Optional[Dict[List[Substitution], List[Substitution]]]
if env is not None:
self.__env = {}
for key, value in env.items():
Expand All @@ -179,12 +179,12 @@ def __init__(
)
self.__log_cmd = log_cmd

self.__process_event_args: Optional[Dict[Text, Any]] = None
self._subprocess_protocol: Optional[Any] = None
self.__process_event_args = None # type: Optional[Dict[Text, Any]]
self._subprocess_protocol = None # type: Optional[Any]
self._subprocess_transport = None
self.__completed_future: Optional[asyncio.Future] = None
self.__sigterm_timer: Optional[TimerAction] = None
self.__sigkill_timer: Optional[TimerAction] = None
self.__completed_future = None # type: Optional[asyncio.Future]
self.__sigterm_timer = None # type: Optional[TimerAction]
self.__sigkill_timer = None # type: Optional[TimerAction]
self.__shutdown_received = False

@property
Expand Down
6 changes: 3 additions & 3 deletions launch/launch/actions/opaque_function.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def __init__(
self, *,
function: Callable,
args: Optional[Iterable[Any]] = None,
kwargs: Optional[Dict[Text, Any]] = None,
kwargs: Optional[Dict[Text, Any]] = None
) -> None:
"""Constructor."""
super().__init__()
Expand All @@ -60,10 +60,10 @@ def __init__(
ensure_argument_type(args, (collections.Iterable, type(None)), 'args', 'OpaqueFunction')
ensure_argument_type(kwargs, (dict, type(None)), 'kwargs', 'OpaqueFunction')
self.__function = function
self.__args: Iterable = []
self.__args = [] # type: Iterable
if args is not None:
self.__args = args
self.__kwargs: Dict[Text, Any] = {}
self.__kwargs = {} # type: Dict[Text, Any]
if kwargs is not None:
self.__kwargs = kwargs

Expand Down
12 changes: 6 additions & 6 deletions launch/launch/actions/timer_action.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
import asyncio
import collections
import logging
from typing import Any
from typing import Any # noqa: F401
from typing import cast
from typing import Dict
from typing import Dict # noqa: F401
from typing import Iterable
from typing import List
from typing import Optional
Expand Down Expand Up @@ -55,7 +55,7 @@ class TimerAction(Action):
def __init__(
self, *,
period: Union[float, SomeSubstitutionsType],
actions: Iterable[LaunchDescriptionEntity],
actions: Iterable[LaunchDescriptionEntity]
Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member Author

@dhood dhood Jun 18, 2018

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, weird. :)

) -> None:
"""Constructor."""
super().__init__()
Expand All @@ -67,10 +67,10 @@ def __init__(
else:
self.__period = normalize_to_list_of_substitutions(period)
self.__actions = actions
self.__context_locals: Dict[Text, Any] = {}
self.__completed_future: Optional[asyncio.Future] = None
self.__context_locals = {} # type: Dict[Text, Any]
self.__completed_future = None # type: Optional[asyncio.Future]
self.__canceled = False
self.__canceled_future: Optional[asyncio.Future] = None
self.__canceled_future = None # type: Optional[asyncio.Future]

async def __wait_to_fire_event(self, context):
done, pending = await asyncio.wait(
Expand Down
2 changes: 1 addition & 1 deletion launch/launch/event_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def __init__(
self,
*,
matcher: Callable[[Event], bool],
entities: Optional[SomeActionsType] = None,
entities: Optional[SomeActionsType] = None
) -> None:
"""
Constructor.
Expand Down
2 changes: 1 addition & 1 deletion launch/launch/event_handlers/on_process_exit.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def __init__(self, *, target_action=None, on_exit) -> None: # noqa: F811
# TODO(wjwwood) check that it is not only callable, but also a callable that matches
# the correct signature for a handler in this case
self.__on_exit = on_exit
self.__actions_on_exit: List[LaunchDescriptionEntity] = []
self.__actions_on_exit = [] # type: List[LaunchDescriptionEntity]
if callable(on_exit):
# Then on_exit is a function or lambda, so we can just call it, but
# we don't put anything in self.__actions_on_exit because we cannot
Expand Down
2 changes: 1 addition & 1 deletion launch/launch/event_handlers/on_process_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def __init__(
target_action: Optional['ExecuteProcess'] = None,
on_stdin: Callable[[ProcessIO], Optional[SomeActionsType]] = None,
on_stdout: Callable[[ProcessIO], Optional[SomeActionsType]] = None,
on_stderr: Callable[[ProcessIO], Optional[SomeActionsType]] = None,
on_stderr: Callable[[ProcessIO], Optional[SomeActionsType]] = None
) -> None:
"""Constructor."""
from ..actions import ExecuteProcess # noqa
Expand Down
2 changes: 1 addition & 1 deletion launch/launch/events/process/process_exited.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def __init__(
*,
returncode: int,
**kwargs
):
) -> None:
"""
Constructor.
Expand Down
2 changes: 1 addition & 1 deletion launch/launch/events/process/process_stdout.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class ProcessStdout(ProcessIO):

name = 'launch.events.process.ProcessStdout'

def __init__(self, *, text: bytes, **kwargs):
def __init__(self, *, text: bytes, **kwargs) -> None:
"""
Constructor.
Expand Down
4 changes: 2 additions & 2 deletions launch/launch/events/process/running_process_event.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def __init__(
cmd: List[Text],
cwd: Optional[Text],
env: Optional[Dict[Text, Text]],
pid: int,
pid: int
) -> None:
"""
Constructor.
Expand Down Expand Up @@ -89,6 +89,6 @@ def env(self) -> Optional[Dict[Text, Text]]:
return self.__env

@property
def pid(self) -> Optional[Dict[Text, Text]]:
def pid(self) -> int:
"""Getter for pid."""
return self.__pid
6 changes: 5 additions & 1 deletion launch/launch/events/process/shutdown_process.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@

from .process_targeted_event import ProcessTargetedEvent

if False:
# imports here would cause loops, but are only used as forward-references for type-checking
from ...actions import ExecuteProcess # noqa


class ShutdownProcess(ProcessTargetedEvent):
"""
Expand All @@ -32,6 +36,6 @@ class ShutdownProcess(ProcessTargetedEvent):

name = 'launch.events.process.ShutdownProcess'

def __init__(self, *, process_matcher: Callable[['ExecuteProcess'], bool]):
def __init__(self, *, process_matcher: Callable[['ExecuteProcess'], bool]) -> None:
"""Constructor."""
super().__init__(process_matcher=process_matcher)
22 changes: 11 additions & 11 deletions launch/launch/launch_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from typing import Any
from typing import Dict
from typing import Iterable
from typing import List
from typing import List # noqa: F401
from typing import Optional
from typing import Text

Expand All @@ -42,20 +42,20 @@ def __init__(self, *, argv: Optional[Iterable[Text]] = None) -> None:
"""
self.__argv = argv if argv is not None else []

self._event_queue: asyncio.Queue = asyncio.Queue()
self._event_handlers: collections.deque = collections.deque()
self._completion_futures: List[asyncio.Future] = []
self._event_queue = asyncio.Queue() # type: asyncio.Queue
self._event_handlers = collections.deque() # type: collections.deque
self._completion_futures = [] # type: List[asyncio.Future]

self.__globals: Dict[Text, Any] = {}
self.__locals_stack: List[Dict[Text, Any]] = []
self.__locals: Dict[Text, Any] = {}
self.__combined_locals_cache: Optional[Dict[Text, Any]] = None
self.__globals = {} # type: Dict[Text, Any]
self.__locals_stack = [] # type: List[Dict[Text, Any]]
self.__locals = {} # type: Dict[Text, Any]
self.__combined_locals_cache = None # type: Optional[Dict[Text, Any]]

self.__launch_configurations_stack: List[Dict[Text, Text]] = []
self.__launch_configurations: Dict[Text, Text] = {}
self.__launch_configurations_stack = [] # type: List[Dict[Text, Text]]
self.__launch_configurations = {} # type: Dict[Text, Text]

self.__is_shutdown = False
self.__asyncio_loop: Optional[asyncio.AbstractEventLoop] = None
self.__asyncio_loop = None # type: Optional[asyncio.AbstractEventLoop]

@property
def argv(self):
Expand Down
36 changes: 21 additions & 15 deletions launch/launch/launch_introspector.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"""Module for the LaunchIntrospector class."""

import logging
from typing import cast
from typing import List
from typing import Text

Expand Down Expand Up @@ -73,7 +74,7 @@ def format_entities(entities: List[LaunchDescriptionEntity]) -> List[Text]:
result = []
for entity in entities:
if is_a(entity, Action):
result.extend(format_action(entity))
result.extend(format_action(cast(Action, entity)))
else:
result.append("Unknown entity('{}')".format(entity))
return result
Expand All @@ -86,11 +87,13 @@ def format_substitutions(substitutions: SomeSubstitutionsType) -> Text:
for sub in normalized_substitutions:
result += ' + '
if is_a(sub, TextSubstitution):
result += "'{}'".format(sub.text)
result += "'{}'".format(cast(TextSubstitution, sub).text)
elif is_a(sub, EnvironmentVariable):
result += 'EnvVarSub({})'.format(format_substitutions(sub.name))
result += 'EnvVarSub({})'.format(
format_substitutions(cast(EnvironmentVariable, sub).name))
elif is_a(sub, FindExecutable):
result += 'FindExecSub({})'.format(format_substitutions(sub.name))
result += 'FindExecSub({})'.format(
format_substitutions(cast(FindExecutable, sub).name))
else:
result += "Substitution('{}')".format(sub)
return result[3:]
Expand All @@ -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
Copy link
Member Author

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.

Copy link
Member

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.

result = [description]
result.extend(indent(format_entities(entities)))
return result
Expand All @@ -111,24 +114,27 @@ def format_event_handler(event_handler: EventHandler) -> List[Text]:
def format_action(action: Action) -> List[Text]:
"""Return a text representation of an action."""
if is_a(action, LogInfo):
return ['LogInfo({})'.format(format_substitutions(action.msg))]
return ['LogInfo({})'.format(format_substitutions(cast(LogInfo, action).msg))]
elif is_a(action, EmitEvent):
return ["EmitEvent(event='{}')".format(action.event.name)]
return ["EmitEvent(event='{}')".format(cast(EmitEvent, action).event.name)]
elif is_a(action, ExecuteProcess):
typed_action = cast(ExecuteProcess, action)
msg = 'ExecuteProcess(cmd=[{}], cwd={}, env={}, shell={})'.format(
', '.join([format_substitutions(x) for x in action.cmd]),
action.cwd if action.cwd is None else "'{}'".format(
format_substitutions(action.cwd)
', '.join([format_substitutions(x) for x in typed_action.cmd]),
typed_action.cwd if typed_action.cwd is None else "'{}'".format(
format_substitutions(typed_action.cwd)
),
action.env if action.env is None else '{' + ', '.join(
typed_action.env if typed_action.env is None else '{' + ', '.join(
['{}: {}'.format(format_substitutions(k), format_substitutions(v))
for k, v in action.env.items()] + '}'),
action.shell,
for k, v in typed_action.env.items()] + '}'),
typed_action.shell,
)
return [msg]
elif is_a(action, RegisterEventHandler):
result = ["RegisterEventHandler('{}'):".format(action.event_handler)]
result.extend(indent(format_event_handler(action.event_handler)))
# Different variable name used to assist with type checking.
typed_action2 = cast(RegisterEventHandler, action)
result = ["RegisterEventHandler('{}'):".format(typed_action2.event_handler)]
result.extend(indent(format_event_handler(typed_action2.event_handler)))
return result
else:
return ["Action('{}')".format(action)]
Expand Down
11 changes: 6 additions & 5 deletions launch/launch/launch_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@
import logging
import threading
from typing import Iterable
from typing import List
from typing import List # noqa: F401
from typing import Optional
from typing import Set
from typing import Set # noqa: F401
from typing import Text
from typing import Tuple
from typing import Tuple # noqa: F401

import osrf_pycommon.process_utils

Expand Down Expand Up @@ -88,7 +88,8 @@ def __init__(
self.__context.register_event_handler(OnShutdown(on_shutdown=self.__on_shutdown))

# Setup storage for state.
self._entity_future_pairs: List[Tuple[LaunchDescriptionEntity, asyncio.Future]] = []
self._entity_future_pairs = \
[] # type: List[Tuple[LaunchDescriptionEntity, asyncio.Future]]

# Used to prevent run() being called from multiple threads.
self.__running_lock = threading.Lock()
Expand Down Expand Up @@ -202,7 +203,7 @@ async def __run_loop(self) -> None:
entity_futures = [pair[1] for pair in self._entity_future_pairs]
entity_futures.append(process_one_event_task)
entity_futures.extend(self.__context._completion_futures)
done: Set[asyncio.Future] = set()
done = set() # type: Set[asyncio.Future]
while not done:
done, pending = await asyncio.wait(
entity_futures,
Expand Down
11 changes: 6 additions & 5 deletions launch/launch/substitutions/launch_configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def __init__(
self,
variable_name: SomeSubstitutionsType,
*,
default: Optional[Union[Any, Iterable[Any]]] = None,
default: Optional[Union[Any, Iterable[Any]]] = None
) -> None:
"""Constructor."""
super().__init__()
Expand All @@ -49,8 +49,8 @@ def __init__(
self.__default = default
else:
# convert any items in default that are not a Substitution or str to a str
str_normalized_default: List[Union[Text, Substitution]] = []
definitely_iterable_default: Iterable[Any] # noqa
str_normalized_default = [] # type: List[Union[Text, Substitution]]
definitely_iterable_default = ((),) # type: Iterable[Any]
if isinstance(default, collections.Iterable):
definitely_iterable_default = default
else:
Expand All @@ -61,8 +61,9 @@ def __init__(
else:
str_normalized_default.append(str(item))
# use normalize_to_list_of_substitutions to convert str to TextSubstitution's too
self.__default: List[Substitution] = \
normalize_to_list_of_substitutions(str_normalized_default)
self.__default = \
normalize_to_list_of_substitutions(
str_normalized_default) # type: List[Substitution]

@property
def variable_name(self) -> List[Substitution]:
Expand Down
2 changes: 1 addition & 1 deletion launch/launch/substitutions/text_substitution.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
class TextSubstitution(Substitution):
"""Substitution that wraps a single string text."""

def __init__(self, *, text: Text):
def __init__(self, *, text: Text) -> None:
"""Constructor."""
super().__init__()

Expand Down
Loading