Skip to content

use regex for wildcard matching#1839

Merged
fujitatomoya merged 7 commits intoros2:rollingfrom
iuhilnehc-ynos:topic-wildcard-use-regex
Jul 6, 2022
Merged

use regex for wildcard matching#1839
fujitatomoya merged 7 commits intoros2:rollingfrom
iuhilnehc-ynos:topic-wildcard-use-regex

Conversation

@iuhilnehc-ynos
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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)

@rpaaron
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

CI:

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

@fujitatomoya
Copy link
Copy Markdown
Collaborator

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

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

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

@fujitatomoya
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 iuhilnehc-ynos force-pushed the topic-wildcard-use-regex branch from 64007f1 to 922179b Compare July 6, 2022 06:00
@iuhilnehc-ynos
Copy link
Copy Markdown
Collaborator Author

Rebased branch and re-run CI:

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

@fujitatomoya
Copy link
Copy Markdown
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
Copy Markdown

eranroll commented Aug 7, 2022

Hi,

Will this be pulled into Galactic or Humble?

@ivanpauno
Copy link
Copy Markdown
Member

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

@fujitatomoya
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

mergify bot commented Aug 10, 2022

backport humble galactic

✅ Backports have been created

Details

@ivanpauno
Copy link
Copy Markdown
Member

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

@fujitatomoya
Copy link
Copy Markdown
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