-
Notifications
You must be signed in to change notification settings - Fork 167
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
Conversation
22ea5d3
to
3a2488b
Compare
There was a problem hiding this 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
ros2param/ros2param/api/__init__.py
Outdated
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) |
There was a problem hiding this comment.
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>
2d46154
to
3e223e4
Compare
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] |
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. |
ros2param/test/test_api.py
Outdated
from ros2param.api import get_parameter_value | ||
|
||
|
||
def test_bool(): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>
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>
@sgvandijk Sorry for the late reply. I applied some minor cleanup:
|
492fba0
to
152f207
Compare
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. |
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