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 capability to use ros2 param set for array types #199

Merged
merged 9 commits into from
Apr 25, 2019

Conversation

sgvandijk
Copy link
Contributor

Using YAML parsing to determine the value parameter type, the supported parameter types are extended to array types. Only the byte array type is excluded, I don't see a way of expressing that in YAML in an unambiguous way.

Fixes #198

@tfoote tfoote added the in review Waiting for review (Kanban column) label Feb 17, 2019
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Please install the additional flake8 plugins mentioned in the instructions and address the resulting linter warnings:

./test/test_api.py:16:1: I100 Import statements are in the wrong order. 'from rcl_interfaces.msg import ParameterType' should be before 'from ros2param.api import get_parameter_value'
from rcl_interfaces.msg import ParameterType
^

./test/test_api.py:62:46: Q000 Remove bad quotes
    value = get_parameter_value(string_value="foo")
                                             ^

./test/test_api.py:64:34: Q000 Remove bad quotes
    assert value.string_value == "foo"
                                 ^

./test/test_api.py:88:41: Q000 Remove bad quotes
    assert value.string_array_value == ["foo", "bar", "buzz"]
                                        ^

./test/test_api.py:88:48: Q000 Remove bad quotes
    assert value.string_array_value == ["foo", "bar", "buzz"]
                                               ^

./test/test_api.py:88:55: Q000 Remove bad quotes
    assert value.string_array_value == ["foo", "bar", "buzz"]
                                                      ^

./ros2param/api/__init__.py:43:12: C407 Unnecessary list comprehension - 'all' can take a generator.
        if all([isinstance(v, bool) for v in yaml_value]):
           ^

./ros2param/api/__init__.py:46:14: C407 Unnecessary list comprehension - 'all' can take a generator.
        elif all([isinstance(v, int) for v in yaml_value]):
             ^

./ros2param/api/__init__.py:49:14: C407 Unnecessary list comprehension - 'all' can take a generator.
        elif all([isinstance(v, float) for v in yaml_value]):
             ^

./ros2param/api/__init__.py:52:14: C407 Unnecessary list comprehension - 'all' can take a generator.
        elif all([isinstance(v, str) for v in yaml_value]):
             ^

4     C407 Unnecessary list comprehension - 'all' can take a generator.
1     I100 Import statements are in the wrong order. 'from rcl_interfaces.msg import ParameterType' should be before 'from ros2param.api import get_parameter_value'
5     Q000 Remove bad quotes

value.bool_value = string_value.lower() == 'true'
elif _is_integer(string_value):
value.bool_value = yaml_value
elif isinstance(yaml_value, int):
value.type = ParameterType.PARAMETER_INTEGER
value.integer_value = int(string_value)
Copy link
Member

Choose a reason for hiding this comment

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

This could assign yaml_value instead which makes the int() call is obsolete.

Same for float below.

Signed-off-by: Sander G. van Dijk <sgvandijk@gmail.com>
Signed-off-by: Sander G. van Dijk <sgvandijk@gmail.com>
Signed-off-by: Sander G. van Dijk <sgvandijk@gmail.com>
Signed-off-by: Sander G. van Dijk <sgvandijk@gmail.com>
Signed-off-by: Sander G. van Dijk <sgvandijk@gmail.com>
Signed-off-by: Sander G. van Dijk <sgvandijk@gmail.com>
@sgvandijk
Copy link
Contributor Author

Thanks for the review! I've applied the requested changes. I've had a look at the failing build, looks not related to this:

ERROR: unable to process source [https://raw.githubusercontent.com/ros/rosdistro/master/releases/fuerte.yaml]

@cottsay
Copy link
Member

cottsay commented Feb 21, 2019

That appears to have been an infrastructure issue with Github that has since been resolved. It was also worked around by ros-infrastructure/rosdep#663.

I'll try to re-trigger the build.

@cottsay cottsay closed this Feb 21, 2019
@cottsay cottsay reopened this Feb 21, 2019
@cottsay cottsay added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in review Waiting for review (Kanban column) in progress Actively being worked on (Kanban column) labels Feb 21, 2019
from ros2param.api import get_parameter_value


def test_bool():
Copy link

Choose a reason for hiding this comment

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

All these tests are quite similar, so we could merge them in a single parametrized test, that is easier to maintain. Something like

import pytest

@pytest.mark.parametrize("string_value,value_attribute,expected_type,expected_value", [
    ('true', 'bool_value', ParameterType.PARAMETER_BOOL, True),
    ('foo', 'string_value', ParameterType.PARAMETER_STRING, 'foo'),
   ('["foo", "bar", "buzz"]', 'string_array_value', ParameterType.PARAMETER_STRING_ARRAY, ['foo', 'bar', 'buzz'])
])
def test_get_parameter_value(string_value, value_attribute, expected_type, expected_value):
    value = get_parameter_value(string_value=string_value)
    assert value.type == expected_type
    assert getattr(value, value_attribute) is expected_value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that, doing that now.

Signed-off-by: Sander G. van Dijk <sgvandijk@gmail.com>
@sgvandijk
Copy link
Contributor Author

Just to check if there are any more blockers for this PR?

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas
Copy link
Member

@sgvandijk Sorry for the late reply. I applied some minor cleanup:

CI is looking good: Build Status

@dirk-thomas
Copy link
Member

Since I made the changes through the web interface the commits were not signed of making the DCO check fail. I took the liberty to force push my last two commits with a proper "Signed-off-by" line.

This is good to be ⛵ now. Thank you for the contribution.

@dirk-thomas dirk-thomas merged commit fe7fb51 into ros2:master Apr 25, 2019
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Apr 25, 2019
esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ros2param] ros2 param set does not work for array types
5 participants