Support namespace and node name wildcards

[rpaaron]

Addresses #1265
I'd like to add tests but adding wildcards to params via arguments returns an error. Alternative is to add a .yml file, but im not sure about adding a filesystem dependency to the tests

[fujitatomoya]

besides inline comment, it would be nice to do test with possible combination.

Alternative is to add a .yml file, but im not sure about adding a filesystem dependency to the tests

rclcpp/rclcpp/test/rclcpp/test_node.cpp

Line 688 in 3270aad

test_resources_path / "test_parameters.yaml").string();

declare_parameter_with_cli_overrides test uses 'test_parameters.yaml, i think that you can refer to.

[rpaaron]

besides inline comment, it would be nice to do test with possible combination.

Alternative is to add a .yml file, but im not sure about adding a filesystem dependency to the tests

rclcpp/rclcpp/test/rclcpp/test_node.cpp

Line 688 in 3270aad

test_resources_path / "test_parameters.yaml").string();

declare_parameter_with_cli_overrides test uses 'test_parameters.yaml, i think that you can refer to.

thats great, thank you kindly

[fujitatomoya]

LGTM, but like to hear from others about design.

[iuhilnehc-ynos]

Even if * without double quotes in YAML is invalid, I still think the node wildcards without (") here should be supported, just like the namespace, such as (/*).

ns:
  /*:           # "*" can be also supported, but I think users like this style.
    ros__parameters:
      node_wild_in_ns: "node_wild_in_ns" 

It should be discussed with the maintainer. rcl_yaml_param_parser should be updated if the above suggestion is accepted.

[jacobperron]

Supporting both /* and "*" makes sense to me 👍

[jacobperron]

I take back my previous comment. Name tokens separated by a YAML colon (:) should be treated as if they are separated with a slash (/). Therefore, the example:

ns:
  /*:
    ros__parameters:
      ...

Should resolve to the name /ns//*, which is invalid. The correct way to write it would be:

/ns/*:
  ros__parameters:
    ...

[clalancette]

The same comment as #1265 (comment) applies to this PR.

[mabelzhang]

@rpaaron Friendly ping. Have you had a chance to look into the design raised in the previous comment?

[jacobperron]

Supporting both /* and "*" makes sense to me 👍

[jacobperron]

I think we should have another test case for matching namespaces with multiple tokens. Following the design doc:

• /* should match exactly one token (e.g. /foo or /bar, but not /foo/bar or /)
• /** should match zero or more tokens (e.g. /foo or /bar or /foo/bar or /)

[rpaaron]

@rpaaron Friendly ping. Have you had a chance to look into the design raised in the previous comment?

thanks @mabelzhang can you advise what I need to change here?

[clalancette]

thanks @mabelzhang can you advise what I need to change here?

Essentially this comment: #1265 (comment) . That is, some updates to the design document so that we are all on the same page.

[fujitatomoya]

@rpaaron can you rebase this on master?

[fujitatomoya]

@rpaaron sorry for the confusion, i need to take back the previous request. could you help the review on #1839 instead, if you are available. (please see #1839 (review))

[fujitatomoya]

I will go ahead to close this one in favor of #1839.
CC: @rpaaron @iuhilnehc-ynos