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

Fix how ros2 param interprets command-line arguments. #684

Merged
merged 2 commits into from
Jan 6, 2022

Conversation

clalancette
Copy link
Contributor

Basically ensure that we are always using the parsed yaml
string, so that users can override types as appropriate.
This makes it possible for users to specify something like:

ros2 param set /node param '!!str off'

for force a string to be "off" as opposed to having YAML
intepret it as a boolean.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

This is still a draft because I'm kind of nervous about the change; this has the potential to break some use-cases that worked before. All of the tests pass, and basic testing here has worked fine for me, but I want to do more testing before marking it as ready.

Fixes #677 (at least, as far as I think we can fix it)

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

i think this makes sense. (probably we would want to have documentation how to override with !!str?)

Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI.

@clalancette
Copy link
Contributor Author

@ros-pull-request-builder retest this please

Basically ensure that we are *always* using the parsed yaml
string, so that users can override types as appropriate.
This makes it possible for users to specify something like:

ros2 param set /node param '!!str off'

for force a string to be "off" as opposed to having YAML
intepret it as a boolean.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
After doing some additional testing, I found a bug in the
code, so I fixed that and added a couple more tests.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette force-pushed the clalancette/fix-param-interpretation branch from f839780 to e1569a4 Compare January 5, 2022 22:40
@clalancette
Copy link
Contributor Author

All right, I found some time to do additional testing. This led to a bug which I fixed, and I added more tests. I'm going to mark this as ready for review and run CI on it.

@clalancette clalancette marked this pull request as ready for review January 5, 2022 22:41
@clalancette
Copy link
Contributor Author

clalancette commented Jan 5, 2022

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor Author

I'm going to go ahead and merge this with green CI. See ros2/ros2_documentation#2212 for documentation about this feature.

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.

ros2 param set string type "Off"
3 participants