Skip to content

Conversation

baconbrot
Copy link
Contributor

This PR exposes StringJoinSubstitution from #843 to the frontend, as suggested by @christophebedard in #843.

Usage

(i.e. with delimiter=. and string components=[https://$(var subdomain), ros, org]):

%YAML 1.2
---
launch:
  - arg:
      name: subdomain
      default: 'wiki'
  - log:
      message: "$(string-join . https://$(var subdomain) ros org)"

results in terminal output

[INFO] [launch.user]: https://wiki.ros.org

@baconbrot baconbrot marked this pull request as draft April 12, 2025 12:32
Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

Other than some minor comments, this looks good.

Would you mind adding tests for frontends, i.e., YAML and XML? See the tests I added to launch_xml and launch_yaml in #802. Also, it would be good to add XML and YAML examples to the Python class docstring.

baconbrot and others added 4 commits April 27, 2025 17:10
@baconbrot baconbrot force-pushed the stringjoinsubstitution_frontend branch from bc7bb1e to 669c430 Compare April 27, 2025 18:12
Signed-off-by: Christian Ruf <[email protected]>
@baconbrot baconbrot marked this pull request as ready for review April 27, 2025 18:30
Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests!

Other than some minor suggestions, this looks great!

Comment on lines +39 to +40
assert isinstance(ld.describe_sub_entities()[0], DeclareLaunchArgument)
assert isinstance(ld.describe_sub_entities()[1], SetLaunchConfiguration)
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought, but given that we have a LaunchDescription here, I think it would be more appropriate to call ld.entities[n]. Same below and for the other tests.

return self.__delimiter

@classmethod
def parse(cls, data: Sequence[SomeSubstitutionsType]):
Copy link
Member

Choose a reason for hiding this comment

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

Type annotation in launch has progressed a lot lately. Could you rebase on rolling and annotate the return type? I think this should do it:

Suggested change
def parse(cls, data: Sequence[SomeSubstitutionsType]):
def parse(
cls, data: Sequence[SomeSubstitutionsType],
) -> Tuple[Type['StringJoinSubstitution'], Dict[str, Any]]:

Comment on lines +107 to +110
kwargs: Dict[str, SomeSubstitutionsType | Iterable[SomeSubstitutionsType]] = {}
kwargs['delimiter'] = data[0]
kwargs['substitutions'] = data[1:]
return cls, kwargs
Copy link
Member

Choose a reason for hiding this comment

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

After the change to the return type annotation, can you try to remove the type annotation for kwargs here and see if mypy is fine with it? Otherwise, you can try to just directly return a dict:

Suggested change
kwargs: Dict[str, SomeSubstitutionsType | Iterable[SomeSubstitutionsType]] = {}
kwargs['delimiter'] = data[0]
kwargs['substitutions'] = data[1:]
return cls, kwargs
return cls, {'delimiter': data[0], 'substitutions': data[1:]}

Co-authored-by: Christophe Bedard <[email protected]>
Signed-off-by: Christian Ruf <[email protected]>
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