Skip to content

Use of -r/--remap flags where appropriate.#59

Merged
hidmic merged 2 commits intomasterfrom
hidmic/use-cli-remap-flags
Sep 3, 2019
Merged

Use of -r/--remap flags where appropriate.#59
hidmic merged 2 commits intomasterfrom
hidmic/use-cli-remap-flags

Conversation

@hidmic
Copy link
Contributor

@hidmic hidmic commented Aug 29, 2019

Precisely what the title says.

Connected to ros2/rcl#491.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@ivanpauno
Copy link
Member

The patch is not correct, you have to modify the local substitutions here:

if self.__node_name is not None:
ros_specific_arguments['name'] = '__node:={}'.format(self.__expanded_node_name)
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))
if self.__expanded_remappings is not None:
ros_specific_arguments['remaps'] = []
for remapping_from, remapping_to in self.__expanded_remappings:
remap_arguments = cast(List[str], ros_specific_arguments['remaps'])
remap_arguments.append(
'{}:={}'.format(remapping_from, remapping_to)
)

@hidmic
Copy link
Contributor Author

hidmic commented Aug 29, 2019

That was actually a tradeoff solution. Functionally it works the same way as if local substitutions had been updated, though it renders ros_specific_arguments incomplete, so much is true. Problem is, substitutions do not expand to List[Text] nor we have support for tokenizing cmd (ros2/launch#263), and I purposefully wanted to keep these changes scoped.

Once ros2/launch#263 is there, yes, we can change this.

@ivanpauno
Copy link
Member

That was actually a tradeoff solution. Functionally it works the same way as if local substitutions had been updated, though it renders ros_specific_arguments incomplete, so much is true. Problem is, substitutions do not expand to List[Text] nor we have support for tokenizing cmd (ros2/launch#263), and I purposefully wanted to keep these changes scoped.

Ok, I understand.
I first thought that the following was possible:

  • '-r' + local substitution added to the command.
  • local substitution is replaced for an empty string, and then remapping rule parsing fails.

But after reading this for a second time, I think that that isn't possible.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic merged commit 8f99605 into master Sep 3, 2019
@delete-merged-branch delete-merged-branch bot deleted the hidmic/use-cli-remap-flags branch September 3, 2019 17:34
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.

2 participants