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

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

Merged
merged 2 commits into from
Sep 3, 2019

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.

None yet

2 participants