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

Make name and namespace mandatory in ComposableNodeContainer, remove deprecated alternatives #189

Merged
merged 1 commit into from
Oct 21, 2020

Conversation

ivanpauno
Copy link
Member

Based on discussion in #185, the name and namespace are mandatory.

I think that the intention was to check that, but it was overlooked when adding support for the deprecated name of the argument.
This delete the deprecated name (they were deprecated before Foxy release), and now it's checked.

…deprecated alternatives

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@jacobperron
Copy link
Member

This partially addresses #123.

@ivanpauno
Copy link
Member Author

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

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

I'm not uber convinced we should be making name and namespace mandatory anywhere BUT we control this container executable so I guess it's fine.

LGTM pending green CI !

@clalancette
Copy link
Contributor

One other thing to note; if we go forward with this, please add a note to https://index.ros.org/doc/ros2/Releases/Release-Galactic-Geochelone/ , since this is a change in behavior.

@ivanpauno
Copy link
Member Author

One other thing to note; if we go forward with this, please add a note to https://index.ros.org/doc/ros2/Releases/Release-Galactic-Geochelone/ , since this is a change in behavior.

In Foxy, it was already mandatory both the name and the namespace.
The only difference is that the error wasn't handled correctly and rcl was showing an error (instead of launch detecting the error), but it was impossible to use the action correctly without passing both the name and namespace explicitly (and not None).

See eloquent code.

@jacobperron
Copy link
Member

If anything, we should add a patch to Foxy to produce a better error message from the ComposableNodeContainer __init__.

@jacobperron
Copy link
Member

See #191

@ivanpauno ivanpauno merged commit 0278d00 into master Oct 21, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/comp-node-container-names branch October 21, 2020 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants