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.

Sign up for GitHub

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

Jump to bottom

Update node name matcher #282

Merged
merged 4 commits into from
Oct 28, 2021
Merged

Update node name matcher #282

merged 4 commits into from
Oct 28, 2021

Conversation

jacobperron
Copy link
Member

This PR does a few things:

  • Makes the slash prefix optional for node name (cd7e0b2)
  • Moves matches_node_name to a more general matchers module (deprecating the old function) (3e2cc1c)
  • Add some tests exercising matches_node_name (479272c)

@jacobperron
Make slash prefix for node name matching optional
cd7e0b2 
The node name must be fully qualified, with a namespace, for matching.
This change makes the slash optional for convenience.
For example, the following two calls are now equivalent:

    matches_node_name('/foo')
    matches_node_name('foo')

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Move node name matcher into matchers module
3e2cc1c 
The matcher function 'matches_node_name' is not specific to lifecycle nodes,
so it makes sense for it to live in a more general module.

Add a deprecation warning to the matcher in the lifecycle module.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Add tests for matcher
479272c 
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron self-assigned this Oct 27, 2021
wjwwood
wjwwood approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

I was thinking maybe launch_ros.events.matchers should be a folder and each matcher should get it's own file, but we can always do that later.

@jacobperron
Copy link
Member Author

I was thinking maybe launch_ros.events.matchers should be a folder and each matcher should get it's own file, but we can always do that later.

I am mimicking launch.event.matchers: https://github.com/ros2/launch/blob/2b828e27b22597ec6a49369e9835462588af209e/launch/launch/events/matchers.py

though, I don't mind moving things into a matchers folder as you propose.
I'd rather put things in the place we want them now to avoid another deprecation cycle :)

@wjwwood
Copy link
Member

wjwwood commented Oct 28, 2021

I don't mind, but we can change that in the future without a deprecation cycle.

@jacobperron
Move into matchers folder
3c49eb4 
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

Moved into matchers folder (3c49eb4).

@jacobperron
Copy link
Member Author

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

@jacobperron jacobperron merged commit 24f0e20 into master Oct 28, 2021
@delete-merged-branch delete-merged-branch bot deleted the jacob/update_node_name_matcher branch October 28, 2021 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

Assignees

@jacobperron jacobperron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jacobperron @wjwwood