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

use regex for wildcard matching #1839

Merged
merged 7 commits into from
Jul 6, 2022

Conversation

iuhilnehc-ynos
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos commented Dec 8, 2021

to fix ros2/rcl#954

My intent is to use regex for supporting some complicated wildcard, such as /**/a/b/*/c/d/*/node.

I think that the same param name in a node of param file parsed by order seems more reasonable than the order(/**, specific_node). Because if there are more wildcards items, such /**/node, /ns/**/node, etc, I don't think users would like to memory these special rules.

Note: I can also use a 'std::set<..,std::less>' to store the node keys(* < / < alpha/num), and then move all the relative items iterator into {specific_node, {}}

Signed-off-by: Chen Lihui lihui.chen@sony.com

@iuhilnehc-ynos
Copy link
Collaborator Author

iuhilnehc-ynos commented Dec 9, 2021

I use your test cases from https://github.com/ros2/rcl/pull/809/files#1280, could you help review this PR? @rpaaron

@iuhilnehc-ynos iuhilnehc-ynos marked this pull request as ready for review December 10, 2021 01:59
@fujitatomoya
Copy link
Collaborator

@iuhilnehc-ynos is this PR for #1265 and ros2/rcl#954? could you add a description about what we are trying fix and related issue?

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.

@iuhilnehc-ynos so you kinda have done rebase for #1280 with some changes, right? (new code for support wildcards and borrowing test cases from #1280)

rclcpp/src/rclcpp/parameter_map.cpp Show resolved Hide resolved
@rpaaron
Copy link
Contributor

rpaaron commented Dec 19, 2021

I use your test cases from ~https://github.com/ros2/rcl/pull/809/files~#1280, could you help review this PR? @rpaaron

Hi! Its been a little while for me, so need to re-familiarize myself with it..
Should we add a couple of basic tests for parameter_map_from(const rcl_params_t * const c_params, const char * node_fqn = nullptr); with a non null node_fqn ?

@iuhilnehc-ynos
Copy link
Collaborator Author

@rpaaron

Should we add a couple of basic tests for parameter_map_from(const rcl_params_t * const c_params, const char * node_fqn = nullptr); with a non null node_fqn ?

Thank you, I'll add some for it.

@fujitatomoya
Copy link
Collaborator

CI:

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

@fujitatomoya
Copy link
Collaborator

@alsora @SteveMacenski could you help us review on this?

@ivanpauno ivanpauno self-requested a review March 8, 2022 17:16
@tonynajjar
Copy link
Contributor

This would be really nice to have, any reason it's blocked?

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Jun 22, 2022

No, we are just waiting for another maintainer's review.

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:21
@alsora
Copy link
Collaborator

alsora commented Jul 2, 2022

Hi, looks good to me.
Sorry for the long wait.
Can we run again CI before merging?

@fujitatomoya
Copy link
Collaborator

CI:

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

Chen Lihui and others added 7 commits July 6, 2022 13:50
Co-authored-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@iuhilnehc-ynos
Copy link
Collaborator Author

Rebased branch and re-run CI:

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

@fujitatomoya
Copy link
Collaborator

@iuhilnehc-ynos thanks for checking on this! all green, i will go ahead to merge this.

@fujitatomoya fujitatomoya merged commit 6dd3a03 into ros2:rolling Jul 6, 2022
@eranroll
Copy link

eranroll commented Aug 7, 2022

Hi,

Will this be pulled into Galactic or Humble?

@ivanpauno
Copy link
Member

@iuhilnehc-ynos @fujitatomoya is there an equivalent PR for rclpy?

@fujitatomoya
Copy link
Collaborator

No i dont think so, I am not sure if the same problem (ros2/rcl#954) can be observed in rclpy.

I think this can be backported to Galactic and Humble since it extends the argument with default value.

CC: @iuhilnehc-ynos

@iuhilnehc-ynos
Copy link
Collaborator Author

iuhilnehc-ynos commented Aug 9, 2022

@ivanpauno

is there an equivalent PR for rclpy?

Not yet. I will open a new PR for it.
Note for me. https://github.com/ros2/rclpy/blob/1d0cc63579cdcd4c923dd7e04c3c60372fb445f3/rclpy/src/rclpy/node.cpp#L348

@fujitatomoya

I think this can be backported to Galactic and Humble

I think so.

@fujitatomoya
Copy link
Collaborator

@Mergifyio backport humble galactic

mergify bot pushed a commit that referenced this pull request Aug 10, 2022
* use regex for wildcard matching

Co-authored-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* use map to process the content of parameter file by order

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add more test cases

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* try to not decrease the performance and make the param win last

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* update node name

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* update document comment

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add more test for parameter_map_from

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Co-authored-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
(cherry picked from commit 6dd3a03)
mergify bot pushed a commit that referenced this pull request Aug 10, 2022
* use regex for wildcard matching

Co-authored-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* use map to process the content of parameter file by order

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add more test cases

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* try to not decrease the performance and make the param win last

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* update node name

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* update document comment

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add more test for parameter_map_from

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Co-authored-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
(cherry picked from commit 6dd3a03)
@mergify
Copy link

mergify bot commented Aug 10, 2022

backport humble galactic

✅ Backports have been created

@ivanpauno
Copy link
Member

@fujitatomoya @iuhilnehc-ynos have the new wildcard rules been documented somewhere?

@fujitatomoya
Copy link
Collaborator

No i do not think so. can you tell us where it is supposed to be, if you have idea?

fujitatomoya pushed a commit that referenced this pull request Sep 9, 2022
* use regex for wildcard matching (#1839)

* use regex for wildcard matching

Co-authored-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* use map to process the content of parameter file by order

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add more test cases

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* try to not decrease the performance and make the param win last

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* update node name

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* update document comment

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add more test for parameter_map_from

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Co-authored-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
(cherry picked from commit 6dd3a03)

* not to break ABI

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Co-authored-by: Chen Lihui <lihui.chen@sony.com>
fujitatomoya pushed a commit that referenced this pull request Sep 9, 2022
* use regex for wildcard matching (#1839)

* use regex for wildcard matching

Co-authored-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* use map to process the content of parameter file by order

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add more test cases

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* try to not decrease the performance and make the param win last

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* update node name

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* update document comment

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

* add more test for parameter_map_from

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Co-authored-by: Aaron Lipinski <aaron.lipinski@roboticsplus.co.nz>
(cherry picked from commit 6dd3a03)

* not to break ABI

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Co-authored-by: Chen Lihui <lihui.chen@sony.com>
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.

Wildcard doesn't assign parameter if the node has namespace
7 participants