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

Adapt to '--ros-args ... [--]'-based ROS args extraction #405

Merged
merged 4 commits into from
Aug 7, 2019

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Aug 5, 2019

Precisely what the title says. Connected to ros2/rcl#477.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Aug 5, 2019

So, I'm having second thoughts about 3256ee2 too. I did it for consistency with rclcpp behaviour but it makes the API somewhat asymmetric compared to arguments provided to rclpy.init().

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/ros-args-extraction branch from 3256ee2 to b58798b Compare August 5, 2019 20:35
@wjwwood
Copy link
Member

wjwwood commented Aug 5, 2019

Again, I don't feel strongly about it, I don't mind the feature as long as it's documented and consistent.

it makes the API somewhat asymmetric compared to arguments provided to rclpy.init().

I don't really think this is an issue (especially if documented). Personally I think of the arguments to rclpy.init() as "argv" which traditionally the command line arguments, even if it does not have to be the command line arguments, and I think of the arguments to the node as "arguments for this node" which implies to me that it is probably a subset of the full command line arguments anyways.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic merged commit 29e4dcd into master Aug 7, 2019
@delete-merged-branch delete-merged-branch bot deleted the hidmic/ros-args-extraction branch August 7, 2019 15:58
@@ -146,7 +146,7 @@ def __init__(
self._parameter_overrides = {}
self._descriptors = {}

if cli_args is not None:
if cli_args is not None and '--ros-args' not in cli_args:
Copy link
Member

Choose a reason for hiding this comment

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

What if someone does:

cli_args=['asd', '--ros-args', ...]

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as in rclcpp, 'asd' will become an unparsed non-ROS arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but that doesn't match the docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, looks like that docstring is outdated though.

Copy link
Member

Choose a reason for hiding this comment

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

So in the case above, asd is a non-ROS argument, and in the following:

cli_args=['asd']

It's a ros argument, because --ros-args is magically prepended.

I prefer a consistent and explicit behavior, i.e.: always add explicitly --ros-args when needed.
But if I'm the only with this feeling, I don't mind if we left this as-is.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

We sort of discuss this here: ros2/rclcpp#816 (comment)

I can see the point of both sides. I would be fine requiring the user to explicitly use --ros-args with the NodeOptions.arguments if you guys think that would be less surprising. I don't expect it to be used in this way very often, so I think it's fine if it's not quite as convenient as it could be with implicitly added --ros-args.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always adding --ros-args is the least surprising alternative, but it is somewhat verbose. I don't have a strong opinion as to which we should prioritize.

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.

4 participants