[roslaunch] Add yaml type for <param> tag #1045
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I dig it. I've wanted something like this for ages. |
wkentaro
force-pushed
the
roslaunch_param_command_type_yaml
branch
from
4105d70 to
ef0dee9
Compare
wkentaro
force-pushed
the
roslaunch_param_command_type_yaml
branch
from
ef0dee9 to
b746f71
Compare
dirk-thomas
reviewed
May 22, 2017
tools/roslaunch/package.xml
Outdated
| @@ -32,6 +32,7 @@ | |||
| <run_depend version_gte="1.13.3">rosunit</run_depend> | |||
|
|
|||
| <test_depend>rosbuild</test_depend> | |||
| <test_depend>rospy</test_depend> | |||
There was a problem hiding this comment.
Why is this necessary? I don't see any part in the patch which requires it.
| @@ -70,30 +70,36 @@ def convert_value(value, type_): | |||
| #attempt numeric conversion | |||
| try: | |||
| if '.' in value: | |||
| return float(value) | |||
| return float(value.strip()) | |||
There was a problem hiding this comment.
This is not necessary. float() already works even if the passed argument has leading / trailing spaces.
Same below.
| except ValueError as e: | ||
| pass | ||
| #bool | ||
| lval = value.lower() | ||
| lval = value.strip().lower() |
There was a problem hiding this comment.
In which case do you expect the strings true and false to have leading / trailing spaces?
| # - lazy import | ||
| global yaml | ||
| if yaml is None: | ||
| import yaml |
There was a problem hiding this comment.
Please move the import to the top of the file and avoid the conditional block.
| global yaml | ||
| if yaml is None: | ||
| import yaml | ||
| return yaml.load(value) |
There was a problem hiding this comment.
How is that represented on the parameter server? Does this store a complex type (e.g. a nested dictionary) under a single name?
There was a problem hiding this comment.
It is set like this:
% rosparam get /test1
bin_contents:
bin_I: [sharpie_accent_tank_style_highlighters, dr_browns_bottle_brush]
work_order:
- {bin: bin_I, item: sharpie_accent_tank_style_highlighters}
% rosparam get /test1/bin_contents/bin_I
[sharpie_accent_tank_style_highlighters, dr_browns_bottle_brush]
% rosparam get /test1/bin_contents
bin_I: [sharpie_accent_tank_style_highlighters, dr_browns_bottle_brush]
% rosparam get /test1/bin_contents/bin_I
[sharpie_accent_tank_style_highlighters, dr_browns_bottle_brush]
dirk-thomas
reviewed
May 24, 2017
| @@ -94,6 +95,8 @@ def convert_value(value, type_): | |||
| elif value == 'false' or value == '0': | |||
| return False | |||
| raise ValueError("%s is not a '%s' type"%(value, type_)) | |||
| elif type_ == 'yaml': | |||
| return yaml.load(value) | |||
There was a problem hiding this comment.
Should any yaml specific exceptions be catched here and a ValueError be raised instead?
|
Had error below: |
wkentaro
force-pushed
the
roslaunch_param_command_type_yaml
branch
from
2d9ecb7 to
c907f36
Compare
|
I am sorry for the late response. This looks good now. Merging... Can you please add appropriate documentation for this new feature to the wiki and link to the change here. Thanks. |
|
Thank you. I reformated the paragraph a little bit and changed the note that it is only available as of Lunar atm: http://wiki.ros.org/action/info/roslaunch/XML/param?action=diff&rev2=12&rev1=11 |
sputnick1124
pushed a commit
to sputnick1124/ros_comm
that referenced
this pull request
Jul 30, 2017
* Add yaml type for <param> tag * Support type for command output of <param> tag * Add test for yaml type * Add test for identifying commandline output as types * Support yaml type with textfile for <param> * Reserve current behavior with handling strip cleanly * Revert <test_depend>rospy</test_depend>: no need * Revert no need strip() * Remove the lazy import for yaml * Raise ValueError for yaml parse error * Strip for boolean rosparam
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why?
To set dict, list string as a parameter.
This is ok if we use
<rosparam>tag, but it does not have the feature of running arbitrary commands which<param>tag has.Example
My motivation is ways to use like below:
% cat apc2015_00.json { "bin_contents": { "bin_I": [ "sharpie_accent_tank_style_highlighters", "dr_browns_bottle_brush" ] }, "work_order": [ { "bin": "bin_I", "item": "sharpie_accent_tank_style_highlighters" } ] } % cat spam.launch <launch> <param name="/test" type="yaml" command="/usr/local/bin/json2yaml $(find roslaunch)/apc2015_00.json" /> </launch> % roslaunch ./spam.launch % rosparam get /test bin_contents: bin_I: [sharpie_accent_tank_style_highlighters, dr_browns_bottle_brush] work_order: - {bin: bin_I, item: sharpie_accent_tank_style_highlighters}