Skip to content

Promote special CLI rules to flags #68

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

Merged
hidmic merged 2 commits into from
Sep 12, 2019
Merged

Promote special CLI rules to flags #68

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6fdd89
Promote special CLI rules to flags.
hidmic Sep 4, 2019
e28cc82
Remove some short form CLI flags.
hidmic Sep 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions launch_ros/launch_ros/actions/node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ def __init__(
# evaluate to paths), or dictionaries of parameters (fields can be substitutions).
i = 0
for param in parameters:
cmd += [LocalSubstitution(
cmd += ['--params-file', LocalSubstitution(
"ros_specific_arguments['params'][{}]".format(i),
description='parameter {}'.format(i))]
i += 1
Expand Down Expand Up @@ -349,10 +349,7 @@ def execute(self, context: LaunchContext) -> Optional[List[Action]]:
if self.__expanded_node_namespace != '':
ros_specific_arguments['ns'] = '__ns:={}'.format(self.__expanded_node_namespace)
if self.__expanded_parameter_files is not None:
ros_specific_arguments['params'] = []
param_arguments = cast(List[str], ros_specific_arguments['params'])
for param_file_path in self.__expanded_parameter_files:
param_arguments.append('__params:={}'.format(param_file_path))
ros_specific_arguments['params'] = self.__expanded_parameter_files
Copy link
Member

@ivanpauno ivanpauno Sep 9, 2019

Choose a reason for hiding this comment

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

IIUC, -f flag only accepts one parameter file, and ros_specific_arguments['params'] (and self.__expanded_parameter_files) is a list.
And the limitation we talked in #59 (comment) isn't true.
Thought substitutions are usually performed to Text (and they should to please the type annotations), we were using LocalSubstitutions that were performed to List[str] or anything else (e.g.: ros_specific_arguments['params']).
See https://github.com/ros2/launch/blob/79975bf453af17fbc2848412d264dbcead623792/launch/launch/substitutions/local_substitution.py#L54.

Copy link
Contributor Author

@hidmic hidmic Sep 9, 2019

Choose a reason for hiding this comment

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

Hmm, a LocalSubstitution can evaluate to anything, that's true -- despite the fact we've type-annotated it as Text. But that's why I think we've been performing substitutions by indexing ros_specific_arguments['params'], and why I simply removed __params:= and prepended -f above.

Copy link
Member

@ivanpauno ivanpauno Sep 9, 2019

Choose a reason for hiding this comment

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

Sorry, I read the patch badly. I missed that we were doing ros_specific_arguments['params'][i] in the local substitution and not ros_specific_arguments['params'].

if self.__expanded_remappings is not None:
ros_specific_arguments['remaps'] = []
for remapping_from, remapping_to in self.__expanded_remappings:
Expand Down