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

Add normalize_remap_rule and types #173

Merged
merged 2 commits into from
Feb 8, 2019
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
8 changes: 4 additions & 4 deletions launch_ros/launch_ros/actions/node.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@
from launch.utilities import normalize_to_list_of_substitutions
from launch.utilities import perform_substitutions

from launch_ros.remap_rule_type import SomeRemapRules
from launch_ros.substitutions import ExecutableInPackage
from launch_ros.utilities import normalize_remap_rules

from rclpy.validate_namespace import validate_namespace
from rclpy.validate_node_name import validate_node_name
Expand All @@ -56,7 +58,7 @@ def __init__(
node_name: Optional[SomeSubstitutionsType] = None,
node_namespace: Optional[SomeSubstitutionsType] = None,
parameters: Optional[List[SomeSubstitutionsType]] = None,
remappings: Optional[List[Tuple[SomeSubstitutionsType, SomeSubstitutionsType]]] = None,
remappings: Optional[SomeRemapRules] = None,
arguments: Optional[Iterable[SomeSubstitutionsType]] = None,
**kwargs
) -> None:
Expand Down Expand Up @@ -148,10 +150,8 @@ def __init__(
description='parameter {}'.format(i))]
ros_args_index += 1
if remappings is not None:
ensure_argument_type(remappings, (list), 'remappings', 'Node')
i = 0
for remapping in remappings:
ensure_argument_type(remapping, (tuple), 'remappings[{}]'.format(i), 'Node')
for remapping in normalize_remap_rules(remappings):
k, v = remapping
i += 1
cmd += [LocalSubstitution(
Expand Down
27 changes: 27 additions & 0 deletions launch_ros/launch_ros/remap_rule_type.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Copyright 2019 Open Source Robotics Foundation, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""Module for remap rule types."""

from typing import Iterable
from typing import Tuple

from launch.some_substitutions_type import SomeSubstitutionsType
from launch.substitution import Substitution


SomeRemapRule = Tuple[SomeSubstitutionsType, SomeSubstitutionsType]
SomeRemapRules = Iterable[SomeRemapRule]
RemapRule = Tuple[Tuple[Substitution, ...], Tuple[Substitution, ...]]
Copy link
Contributor Author

@sloretz sloretz Feb 1, 2019

Choose a reason for hiding this comment

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

This is Tuple[Tuple[ instead of Tuple[List[ to make a normalized remap rule immutable. (Edit: well, the Substition instances in the Tuple aren't immutable, so I guess it's just less mutable)

There isn't anything called description yet, so currently I'm thinking a launch_ros.description.ComposableNode instance is an immutable description of some end state. Unlike an action instance which is only performed once, a description could be given to multiple actions. Defining the type as Tuple is a way to prevent remap rules on it from being modified.

Copy link
Member

Choose a reason for hiding this comment

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

Re: Tuple vs List, immutable is fine by me.

Re: launch_ros.description.*, is there a description base class for these? If not, I had imagined they would not be in a subpackage together. If so, then I think a module called launch_ros.description with a subpackage called launch_ros.descriptions.* would be most appropriate.

Otherwise, I think launch_ros.ComposableNode is just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re: launch_ros.description.*, is there a description base class for these? If not, I had imagined they would not be in a subpackage together.

Roger, launch_ros.ComposableNode it is

RemapRules = Iterable[RemapRule]
28 changes: 28 additions & 0 deletions launch_ros/launch_ros/utilities/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Copyright 2019 Open Source Robotics Foundation, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""
Module for descriptions of launchable entities.

Descriptions are not executable and are immutable so they can be reused by launch entities.
"""

from .normalize_remap_rule import normalize_remap_rule
from .normalize_remap_rule import normalize_remap_rules


__all__ = [
'normalize_remap_rule',
'normalize_remap_rules',
]
39 changes: 39 additions & 0 deletions launch_ros/launch_ros/utilities/normalize_remap_rule.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Copyright 2019 Open Source Robotics Foundation, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""Module for normalize_remap_rule utility."""

from launch.utilities import ensure_argument_type
from launch.utilities import normalize_to_list_of_substitutions

from ..remap_rule_type import RemapRule
from ..remap_rule_type import RemapRules
from ..remap_rule_type import SomeRemapRule
from ..remap_rule_type import SomeRemapRules


def normalize_remap_rule(remap_rule: SomeRemapRule) -> RemapRule:
"""Normalize a remap rule to a specific type."""
ensure_argument_type(remap_rule, (tuple), 'remap_rule')
if len(remap_rule) != 2:
raise TypeError(
'remap_rule must be a tuple of length 2, got length {}'.format(len(remap_rule)))
from_sub = tuple(normalize_to_list_of_substitutions(remap_rule[0]))
to_sub = tuple(normalize_to_list_of_substitutions(remap_rule[1]))
return from_sub, to_sub


def normalize_remap_rules(remap_rules: SomeRemapRules) -> RemapRules:
"""Normalize multiple remap rules."""
yield from map(normalize_remap_rule, remap_rules)
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# Copyright 2019 Open Source Robotics Foundation, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""Tests for the Node Action."""

from launch import LaunchContext
from launch.substitutions import TextSubstitution
from launch.utilities import perform_substitutions
from launch_ros.utilities import normalize_remap_rule
from launch_ros.utilities import normalize_remap_rules
import pytest


def test_not_a_tuple():
with pytest.raises(TypeError):
normalize_remap_rule(['foo', 'bar'])


def test_wrong_rule_length():
with pytest.raises(TypeError):
normalize_remap_rule(('foo'))

with pytest.raises(TypeError):
normalize_remap_rule(('foo', 'bar', 'baz'))


def test_plain_text():
lc = LaunchContext()
rule = ('foo', 'bar')
output_rule = normalize_remap_rule(rule)
assert isinstance(output_rule, tuple)
assert len(output_rule) == 2
assert rule[0] == perform_substitutions(lc, output_rule[0])
assert rule[1] == perform_substitutions(lc, output_rule[1])


def test_mixed_substitutions():
lc = LaunchContext()
rule = (('foo', 'bar'), ['bar', TextSubstitution(text='baz')])
output_rule = normalize_remap_rule(rule)
assert isinstance(output_rule, tuple)
assert len(output_rule) == 2
assert 'foobar' == perform_substitutions(lc, output_rule[0])
assert 'barbaz' == perform_substitutions(lc, output_rule[1])


def test_multiple_rules():
lc = LaunchContext()
rules = [('ping', 'pong'), (('baz', 'foo'), ['bar', TextSubstitution(text='baz')])]
output_rules = list(normalize_remap_rules(rules))
assert len(rules) == len(output_rules)
assert 'ping' == perform_substitutions(lc, output_rules[0][0])
assert 'pong' == perform_substitutions(lc, output_rules[0][1])
assert 'bazfoo' == perform_substitutions(lc, output_rules[1][0])
assert 'barbaz' == perform_substitutions(lc, output_rules[1][1])