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.

Sign up for GitHub

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

More Helpful Error Messages (backport #275) #289

Merged
merged 1 commit into from
Dec 2, 2021
Merged

More Helpful Error Messages (backport #275) #289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 15 additions & 6 deletions launch_ros/launch_ros/utilities/evaluate_parameters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,17 @@ def check_sequence_type_is_allowed(sequence):
if isinstance(value[0], Substitution):
# Value is a list of substitutions, so perform them to make a string
evaluated_value = perform_substitutions(context, list(value))
yaml_evaluated_value = yaml.safe_load(evaluated_value)
if yaml_evaluated_value is None:
yaml_evaluated_value = ''
Copy link
Member

Choose a reason for hiding this comment

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

Removing this check breaks existing launch files in Foxy (and Galactic). For example, the following used to work:

import launch
from launch.actions import DeclareLaunchArgument
from launch.substitutions import LaunchConfiguration
from launch_ros.actions import Node


def generate_launch_description():
    return launch.LaunchDescription([
        DeclareLaunchArgument('foo', default_value=''),
        Node(
            package='demo_nodes_py',
            executable='talker',
            name='talker_node',
            output='screen',
            parameters = [
                {'foo': LaunchConfiguration('foo')}
            ]
        ),
    ])

because the LaunchConfiguration substitution evaluated to None which used to translate to the empty string.

I think we can easily fix the issue in Foxy and Galactic by adding back L73-74 above, but I'm not sure what the correct behavior should be on Rolling.

@adityapande-1995 @DLu thoughts?

cc/ @Yadunund

Copy link
Member

Choose a reason for hiding this comment

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

See #300 for a proposed fix.


try:
yaml_evaluated_value = yaml.safe_load(evaluated_value)
except yaml.YAMLError:
raise TypeError(
'Unable to parse the value of parameter {} as yaml. '
'If the parameter is meant to be a string, try wrapping it in '
'launch_ros.parameter_descriptions.ParameterValue'
'(value, value_type=str)'.format(evaluated_name)
)

if type(yaml_evaluated_value) in (bool, int, float, str, bytes):
evaluated_value = yaml_evaluated_value
elif isinstance(yaml_evaluated_value, Sequence):
Expand All @@ -86,9 +94,10 @@ def check_sequence_type_is_allowed(sequence):
else:
raise TypeError(
'Allowed value types are bytes, bool, int, float, str, Sequence[bool]'
', Sequence[int], Sequence[float], Sequence[str]. Got {}.'.format(
type(yaml_evaluated_value)
)
', Sequence[int], Sequence[float], Sequence[str]. Got {}.'
'If the parameter is meant to be a string, try wrapping it in '
'launch_ros.parameter_descriptions.ParameterValue'
'(value, value_type=str)'.format(type(yaml_evaluated_value))
)
elif isinstance(value[0], Sequence):
# Value is an array of a list of substitutions
Expand Down
57 changes: 57 additions & 0 deletions test_launch_ros/test/test_launch_ros/utilities/test_normalize_parameters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@
"""Tests for the normalizing parameters utility."""

import pathlib
from typing import List

from launch import LaunchContext
from launch.substitutions import TextSubstitution

from launch_ros.parameter_descriptions import ParameterValue
from launch_ros.utilities import evaluate_parameters
from launch_ros.utilities import normalize_parameters

Expand Down Expand Up @@ -339,3 +341,58 @@ def test_unallowed_yaml_types_in_substitutions():
norm = normalize_parameters(orig)
evaluate_parameters(LaunchContext(), norm)
assert 'Expected a non-empty sequence' in str(exc.value)

with pytest.raises(TypeError) as exc:
orig = [{'foo': 1, 'fiz': TextSubstitution(text='Text That : Cannot Be Parsed As : Yaml')}]
norm = normalize_parameters(orig)
evaluate_parameters(LaunchContext(), norm)
assert 'Unable to parse' in str(exc.value)


def test_unallowed_yaml_types_as_strings():
# All the tests from test_unallowed_yaml_types_in_substitutions
# but coerced to the proper type with ParameterValue
orig = [{'foo': 1, 'fiz': ParameterValue(TextSubstitution(text="{'asd': 3}"), value_type=str)}]
norm = normalize_parameters(orig)
expected = ({'foo': 1, 'fiz': "{'asd': 3}"},)
assert evaluate_parameters(LaunchContext(), norm) == expected

orig = [{'foo': 1, 'fiz': ParameterValue(TextSubstitution(text='[1, 2.0, 3]'),
value_type=str)}]
norm = normalize_parameters(orig)
evaluate_parameters(LaunchContext(), norm)
expected = ({'foo': 1, 'fiz': '[1, 2.0, 3]'},)
assert evaluate_parameters(LaunchContext(), norm) == expected

orig = [{'foo': 1, 'fiz': ParameterValue(TextSubstitution(text='[[2, 3], [2, 3], [2, 3]]'),
value_type=str)}]
norm = normalize_parameters(orig)
evaluate_parameters(LaunchContext(), norm)
expected = ({'foo': 1, 'fiz': '[[2, 3], [2, 3], [2, 3]]'},)
assert evaluate_parameters(LaunchContext(), norm) == expected

orig = [{'foo': 1, 'fiz': ParameterValue(TextSubstitution(text='[]'), value_type=str)}]
norm = normalize_parameters(orig)
evaluate_parameters(LaunchContext(), norm)
expected = ({'foo': 1, 'fiz': '[]'},)
assert evaluate_parameters(LaunchContext(), norm) == expected

orig = [{
'foo': 1,
'fiz': ParameterValue([
[TextSubstitution(text="['asd', 'bsd']")],
[TextSubstitution(text="['asd', 'csd']")]
], value_type=List[str])
}]
norm = normalize_parameters(orig)
evaluate_parameters(LaunchContext(), norm)
expected = ({'foo': 1, 'fiz': ["['asd', 'bsd']", "['asd', 'csd']"]},)
assert evaluate_parameters(LaunchContext(), norm) == expected

orig = [{'foo': 1,
'fiz': ParameterValue(TextSubstitution(text='Text That : Cannot Be Parsed As : Yaml'),
value_type=str)}]
norm = normalize_parameters(orig)
evaluate_parameters(LaunchContext(), norm)
expected = ({'foo': 1, 'fiz': 'Text That : Cannot Be Parsed As : Yaml'},)
assert evaluate_parameters(LaunchContext(), norm) == expected