Fix problems when parsing a `Command` `Substitution` as a parameter value by ivanpauno · Pull Request #137 · ros2/launch_ros

ivanpauno · 2020-03-24T21:06:34Z

Trying to address #136.
Related with #74.

Since #31, substitutions in parameter values are loaded like yaml. This is an issue with the Command substitution, because it is not possible to skip the yaml.load() done by the Node action when parsing the input parameters.

My proposal here is to add a Parameter class, which allows to specify the type of the parameter.
One of the options is to convert the result of the substitution like yaml.

Other things I would like to revisit after this:

Add a type attribute in launch frontend parameters, map them to the new API.
Maybe stop supporting to pass a dictionary in parameters Node argument. Or maybe, don't allow substitutions in them (How to tic-toc?).
Consolidate type_utils here with type_utils used in launch.frontend.
Drop support of union of types in launch.frontend.type_utils. Update parsing functions if necessary.
Revisit lists in XML frontend. Drop *-sep and support list written like in yaml?

wjwwood

Seems reasonable to me, but I just glanced over the description and the new API. Still needs a thorough review from someone else.

test_launch_ros/test/test_launch_ros/actions/test_node.py

launch_ros/launch_ros/parameter.py

wjwwood · 2020-03-24T21:32:28Z

Maybe stop supporting to pass a dictionary in parameters Node argument. Or maybe, don't allow substitutions in them (How to tic-toc?).

My instinct is to leave them, but I don't know for sure.

hidmic

I think that conceptually this makes sense, though I don't love how nested parameters end up being specified. I left some comments.

launch_ros/launch_ros/actions/node.py

launch_ros/launch_ros/parameter.py

launch_ros/launch_ros/parameters_type.py

launch_ros/launch_ros/utilities/type_utils.py

test_launch_ros/test/test_launch_ros/utilities/test_type_utils.py

ivanpauno · 2020-06-30T14:35:36Z

Rebased with master, I'm working in addressing comments.

ivanpauno · 2020-07-13T13:41:16Z

I've created ros2/launch#438, to consolidate all type_utils in launch repo.
I still have to polish things up, but this is ready for a review again.

launch_ros/launch_ros/actions/node.py

test_launch_ros/test/test_launch_ros/actions/test_node.py

hidmic · 2020-07-30T22:43:03Z

I have to review this, and we need to decide what to do with

launch_ros/test_launch_ros/test/test_launch_ros/frontend/test_node_frontend.py

Line 50 in 1f0ea9f

    
                               <!-- TODO(hidmic): revisit after https://github.com/ros2/launch_ros/pull/137 gets merged -->

…r type. Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno · 2020-08-03T18:00:31Z

I have to review this, and we need to decide what to do with

launch_ros/test_launch_ros/test/test_launch_ros/frontend/test_node_frontend.py

Line 50 in 1f0ea9f

Those "qoute" can be avoided by explicitly passing the type.

hidmic · 2020-08-03T18:06:12Z

Those "qoute" can be avoided by explicitly passing the type.

Yes, that's what I thought. Though I wonder if we should keep that test case to make sure XML special characters work.

launch_ros/launch_ros/parameter_descriptions.py

hidmic · 2020-08-03T18:19:35Z

test_launch_ros/test/test_launch_ros/frontend/test_node_frontend.py

-                    <param name="param12" value=""/>
+                    <param name="param12" value="''"/>
+                    <param name="param13" value="$(var my_value)" type="str"/>
+                    <param name="param14" value="'2', '5', '8'" value-sep=", " type="list_of_str"/>


@ivanpauno should it be

Suggested change

<param name="param14" value="'2', '5', '8'" value-sep=", " type="list_of_str"/>

<param name="param14" value="2, 5, 8" value-sep=", " type="list_of_str"/>

instead, to actually make sure YAML rules won't get in the way? Same below.

the expected output is ["'2'", "'5'", "'8'"], this is checking that YAML rules aren't used in the way.

Checking that YAML rules are not used to parse a list, but not checking that YAML rules are not used for each element.

Checking that YAML rules are not used to parse a list, but not checking that YAML rules are not used for each element.

The first element is "'2'". If yaml rules were being used, the output would be '2'. This is checking that the output is "'2'".
If I make the change you ask for, the test won't pass.

If I make the change you ask for, the test won't pass.

How so? Assuming both markup snippets and test expectations are adjusted, of course.

I don't mind keeping that expectation as-is. But I'd really like to make sure that:

<param name="test" value="1, 2, 3" value-sep=", " type="list_of_str"/>

effectively yields a list of strings.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>

ivanpauno · 2020-08-03T18:49:05Z

Yes, that's what I thought. Though I wonder if we should keep that test case to make sure XML special characters work.

That's still being tested, I've only deleted the TODOs.

ivanpauno · 2020-08-03T18:50:47Z

I've to re-release launch to make the Rpr checker happy.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno · 2020-08-04T17:30:11Z

@ros-pull-request-builder retest this please

ivanpauno · 2020-08-04T18:42:55Z

@ros-pull-request-builder retest this please

ivanpauno · 2020-08-04T18:59:04Z

CI:

Linux
Linux-aarch64
macOS
Windows

…alue (#137) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno added the in review label Mar 24, 2020

ivanpauno requested review from hidmic and wjwwood March 24, 2020 21:06

ivanpauno self-assigned this Mar 24, 2020

wjwwood reviewed Mar 24, 2020

View reviewed changes

test_launch_ros/test/test_launch_ros/actions/test_node.py Outdated Show resolved Hide resolved

launch_ros/launch_ros/parameter.py Outdated Show resolved Hide resolved

hidmic reviewed Apr 20, 2020

View reviewed changes

ivanpauno mentioned this pull request Jun 25, 2020

Add a SetParameter action that sets a parameter to all nodes in the same scope #158

Merged

ivanpauno force-pushed the ivanpauno/fix-parameter-bad-interaction-frontend branch from a4e1e54 to b2fd4d8 Compare June 30, 2020 14:33

ivanpauno mentioned this pull request Jul 13, 2020

Consolidate type_utils in a way that can be reused in substitution results that need to be coerced to a specific type ros2/launch#438

Merged

ivanpauno requested a review from hidmic July 13, 2020 13:41

hidmic reviewed Jul 20, 2020

View reviewed changes

launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved

test_launch_ros/test/test_launch_ros/actions/test_node.py Outdated Show resolved Hide resolved

ivanpauno requested a review from hidmic July 23, 2020 14:19

hidmic mentioned this pull request Jul 30, 2020

Drop double single-quoted params. #164

Merged

ivanpauno added 12 commits August 3, 2020 14:42

Add a parameter description class that allows specifying the paramete…

254d3c6

…r type. Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Add class for a parameter value description

cff0cd8

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Make use of the new launch_ros.descriptions.ParameterValue

f29664b

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

wip

55f0a8a

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Consolidated with type_utils in launch

b513a70

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Fix how parameters are parsed

6c0f11e

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Fix test expectations

30131da

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Correct bug/typo

e6b1e41

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Restore yaml.FullLoader in test case

331a68d

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Write tests for ParameterValue

5dab4d5

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Test explicit parameter type in launch XML

77ea392

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Add extra test case

4fea16f

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno force-pushed the ivanpauno/fix-parameter-bad-interaction-frontend branch from ba69703 to 4fea16f Compare August 3, 2020 17:59

hidmic approved these changes Aug 3, 2020

View reviewed changes

Improve docblock

42acb9f

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>

ivanpauno requested a review from hidmic August 3, 2020 18:50

hidmic approved these changes Aug 3, 2020

View reviewed changes

Add test case

6313b54

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

hidmic approved these changes Aug 4, 2020

View reviewed changes

ivanpauno merged commit 9250313 into master Aug 4, 2020

delete-merged-branch bot deleted the ivanpauno/fix-parameter-bad-interaction-frontend branch August 4, 2020 21:04

This was referenced Aug 5, 2020

Command substitution: special characters inside XML comments causes launch to fail #136

Closed

Bad interaction between launch frontend and YAML parameters #74

Closed

djchopp mentioned this pull request May 4, 2021

Evaluate parameters exception loading URDF #214

Closed

jacobperron mentioned this pull request Sep 10, 2021

[foxy] Backport parser fixes and type_utils refactor ros2/launch#530

Merged

jacobperron pushed a commit that referenced this pull request Sep 10, 2021

Fix problems when parsing a Command Substitution as a parameter v…

c98e16b

…alue (#137) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

jacobperron mentioned this pull request Sep 10, 2021

[foxy] Fix problems when parsing a Command Substitution as a parameter value (#137) #265

Merged

christophebedard mentioned this pull request Nov 15, 2021

"Building a Visual Robot Model with URDF from Scratch" doesn't work on Foxy ros2/ros2_documentation#2124

Closed

jacobperron pushed a commit that referenced this pull request Nov 16, 2021

Fix problems when parsing a Command Substitution as a parameter v…

7878f41

…alue (#137) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

	<param name="param14" value="'2', '5', '8'" value-sep=", " type="list_of_str"/>
	<param name="param14" value="2, 5, 8" value-sep=", " type="list_of_str"/>

Conversation

ivanpauno commented Mar 24, 2020

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wjwwood commented Mar 24, 2020

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivanpauno commented Jun 30, 2020

Uh oh!

ivanpauno commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hidmic commented Jul 30, 2020

Uh oh!

ivanpauno commented Aug 3, 2020

Uh oh!

hidmic commented Aug 3, 2020

Uh oh!

Uh oh!

hidmic Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

ivanpauno Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

hidmic Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

ivanpauno Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanpauno commented Aug 3, 2020

Uh oh!

ivanpauno commented Aug 3, 2020

Uh oh!

ivanpauno commented Aug 4, 2020

Uh oh!

ivanpauno commented Aug 4, 2020

Uh oh!

ivanpauno commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ivanpauno commented Jul 13, 2020 •

edited

Loading

ivanpauno Aug 4, 2020 •

edited

Loading

hidmic Aug 4, 2020 •

edited

Loading