Support namespace and node name wildcards#1280
Conversation
da57c05 to
e5c0208
Compare
fujitatomoya
left a comment
There was a problem hiding this comment.
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
declare_parameter_with_cli_overrides test uses 'test_parameters.yaml, i think that you can refer to.
d7937c3 to
c986bed
Compare
thats great, thank you kindly |
fujitatomoya
left a comment
There was a problem hiding this comment.
LGTM, but like to hear from others about design.
c986bed to
4eb68a8
Compare
| namespace_wild: "namespace_wild" | ||
|
|
||
| ns: | ||
| "*": |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Supporting both /* and "*" makes sense to me 👍
There was a problem hiding this comment.
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:
...|
The same comment as #1265 (comment) applies to this PR. |
|
@rpaaron Friendly ping. Have you had a chance to look into the design raised in the previous comment? |
| namespace_wild: "namespace_wild" | ||
|
|
||
| ns: | ||
| "*": |
There was a problem hiding this comment.
Supporting both /* and "*" makes sense to me 👍
| ros__parameters: | ||
| full_wild: "full_wild" | ||
|
|
||
| /*: |
There was a problem hiding this comment.
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./fooor/bar, but not/foo/baror/)/**should match zero or more tokens (e.g./fooor/baror/foo/baror/)
Signed-off-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
Signed-off-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
Signed-off-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
4eb68a8 to
957432a
Compare
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. |
|
@rpaaron can you rebase this on master? |
|
@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)) |
|
I will go ahead to close this one in favor of #1839. |
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