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

Describe wildcard usage in parameter files #303

Merged
merged 2 commits into from
Dec 17, 2021

Conversation

jacobperron
Copy link
Member

Wildcards were introduced in ros2/rclcpp#762
And further usage proposed in ros2/rclcpp#1265.

Wildcards were introduced in ros2/rclcpp#762
And further usage proposed in ros2/rclcpp#1265.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Both additions seem like reasonable options (matching all nodes in a namespace, matching all nodes with a name).

I'm not sure if this really has many use cases or not.
If not, I wouldn't add it.

articles/160_ros_command_line_arguments.md Outdated Show resolved Hide resolved
articles/160_ros_command_line_arguments.md Outdated Show resolved Hide resolved
@hidmic
Copy link
Contributor

hidmic commented Oct 13, 2020

Overall LGTM, but I will say that along with ros2/rclcpp#762, ros2/rclpy#370 was introduced. Whatever changes are proposed have to be applied to both client libraries IMHO.

@jacobperron
Copy link
Member Author

Whatever changes are proposed have to be applied to both client libraries

I'll follow-up to make sure that this happens.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

IMO, it would be nicer if the implementation for wildcard matching was done in rcl, then the client libraries would get it for free.

@ivanpauno
Copy link
Member

IMO, it would be nicer if the implementation for wildcard matching was done in rcl, then the client libraries would get it for free.

I think that's possible, it only requires a refactor of the current implementation.

@hidmic
Copy link
Contributor

hidmic commented Oct 14, 2020

I think that's possible, it only requires a refactor of the current implementation.

Hmm, I think I fiddled with it some time ago. Or @Lobotuerk did. We require extra API, as the wildcard matching cannot happen until we have the fully qualified node name.

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

@ivanpauno
Copy link
Member

Checking ros2/rcl#809, I have some doubts if some things are valid or not:

  1. /*
  2. /asd/*/bsd
  3. /asd/**/bsd
  4. /*asd
  5. /asd*
  6. /a*sd
  7. /**asd
  8. /asd**
  9. /a**sd

4 to 9 should be all invalid IMO, * and ** should always be between slashes.
1 should work, because /asd/* works (and I don't think namespace / should be special).
2, 3 also sound like reasonable things to support, but I'm not sure if they are really useful or not.

@jacobperron
Copy link
Member Author

@ivanpauno I completely agree with all your examples. IIUC, the logic added to rcl in ros2/rcl#809 does consider partial matching invalid (e.g. examples 4 to 9).

@jacobperron
Copy link
Member Author

It might be the case that example 1 isn't considered valid in ros2/rcl#809, I'd have to check.

@hidmic
Copy link
Contributor

hidmic commented Feb 8, 2021

It might be the case that example 1 isn't considered valid in ros2/rcl#809, I'd have to check.

We should follow-up.

@iuhilnehc-ynos
Copy link
Contributor

@ivanpauno @jacobperron

I didn't notice your comment before. #303 (comment)

ros2/rcl#809 expects items from 4 to 9 to be invalid (EXPECT_FALSE(res) with the rcl_parse_yaml_file).
image

Am I missing something?

@fujitatomoya
Copy link
Collaborator

@iuhilnehc-ynos i think everything is covered as expected on #303 (comment).

@jacobperron @ivanpauno i will go ahead to merge this. if anything is missing, let us know!

@fujitatomoya fujitatomoya merged commit e160af4 into gh-pages Dec 17, 2021
@delete-merged-branch delete-merged-branch bot deleted the jacob/yaml_wildcards branch December 17, 2021 19:11
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.

5 participants