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

Add PushRosNamespace action #42

Merged
merged 3 commits into from
Jul 19, 2019
Merged

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Jul 18, 2019

Based on discussion ros2/design#207 (review), ros2/design#207 (comment), and inspired on Node action documentation (of a feature that was documented but not implemented).

This adds a way of scoping ROS entities in a namespace.

Example usage:

LaunchDescription([
    GroupAction([
        PushRosNamespace('my_ns'),
        IncludeLaunchDescription(PythonLaunchDescriptionSource('another_launch_file'),
    ])
])

I think we should consider in the future:

  • Add GroupAction and IncludeLaunchDescription alternatives in launch_ros. Add to them an extra ros_namespace argument.
  • Add scoped argument to IncludeLaunchDescription action (in launch), default to True (which will break api, but I think is more "normal").

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

@wjwwood
Copy link
Member

wjwwood commented Jul 18, 2019

Add GroupAction and IncludeLaunchDescription alternatives in launch_ros. Add to them an extra ros_namespace argument.

That is fine by me, though we have to address the namespacing in XML or instead of having new versions that "overload", find a way to extend the actions that are already in launch via launch_ros.

Add scoped argument to IncludeLaunchDescription action (in launch), default to True (which will break api, but I think is more "normal").

I think it would be ok to do this at the XML level, but I would not do this at the action level. I intentionally left scoping to groups as to not complicate every action that might benefit from scoping, like the include action and the set launch configuration action.

@wjwwood
Copy link
Member

wjwwood commented Jul 18, 2019

I would also not change the default behavior of scoping in the Python API, but again, I think that could be appropriate in the XML syntactic sugar.

@ivanpauno
Copy link
Member Author

CI (up to test_launch_ros, launch_ros):

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

Signed-off-by: ivanpauno <[email protected]>
@ivanpauno
Copy link
Member Author

ivanpauno commented Jul 19, 2019

Double checking if linter is happy after 5b64bd7:

  • Linux Build Status

@ivanpauno ivanpauno merged commit 68337f8 into master Jul 19, 2019
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/add-push-ros-namespace branch July 19, 2019 18:27
@ivanpauno ivanpauno restored the ivanpauno/add-push-ros-namespace branch August 2, 2019 13:13
@ivanpauno ivanpauno deleted the ivanpauno/add-push-ros-namespace branch August 2, 2019 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants