Skip to content

Add StringJoinSubstitution substituion#843

Merged
christophebedard merged 6 commits intoros2:rollingfrom
baconbrot:stringjoinsubstitution
Apr 6, 2025
Merged

Add StringJoinSubstitution substituion#843
christophebedard merged 6 commits intoros2:rollingfrom
baconbrot:stringjoinsubstitution

Conversation

@baconbrot
Copy link
Copy Markdown
Contributor

Add a substitution wrapper for python str.join method.
Supports concatenation of substitutions as implemented in #838

This PR would complement PathJoinSubstitution for general use cases like file names with separators (e.g. some.long.urdf.xacro), naming of entities (e.g. the_name_of_the_robot) or even namespaces (e.g. /env/group/robot/joint)

Usecases

Domains
e.g. https://docs.ros.org with subdomain launch configuration equal to docs

StringJoinSubstitution(
    [
        ['https://', LaunchConfiguration('subdomain')],
        'ros',
        'org'
    ],
    delimiter='.'
)

File names
e.g. robot.a.urdf.xacro with model launch configuration equal to a

StringJoinSubstitution(
    [
        'robot'
        LaunchConfiguration('model'),
        'urdf',
        'xacro'
    ],
    delimiter='.'
)

Signed-off-by: Christian Ruf <c.ruf99@gmail.com>

Support concatenation of substitutions as implemented in ros2#838

Signed-off-by: Christian Ruf <c.ruf99@gmail.com>

Add docstrings

Signed-off-by: Christian Ruf <c.ruf99@gmail.com>

Updatre repr

Signed-off-by: Christian Ruf <c.ruf99@gmail.com>

Fix formatting

Signed-off-by: Christian Ruf <c.ruf99@gmail.com>
@christophebedard christophebedard self-requested a review March 27, 2025 17:06
@christophebedard christophebedard self-assigned this Mar 27, 2025
Copy link
Copy Markdown
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.

This seems fine, but could you give a real-world example? Ah, sorry, I glossed over the last example in the PR description, although I kind of think it could simply be ['robot.', LaunchConfiguration('model'), '.urdf.xacro'].

Also, do you think we should support frontends, i.e., by implementing parse()?

Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Christian Ruf <c.ruf99@gmail.com>
@baconbrot
Copy link
Copy Markdown
Contributor Author

baconbrot commented Mar 31, 2025

@christophebedard frontend support would definitly be a great addition for #838 because of cross-platform path handling.

For both cases, PathJoinSubstitution and StringJoinSubstitution, I come across following problem:
Consider previous example for python launch

StringJoinSubstitution(
    [
        'robot'
        LaunchConfiguration('model'),
        'urdf',
        'xacro'
    ],
    delimiter='.'
)

I dont know how this would translate to yaml launch. Maybe to something like this

- arg:
  - name: "model"
- join-string:
      delimiter: "."
        children:
          - "robot"
          - "$(var model)"
          - "urdf"
          - "xacro"

but the join-string result is not accessible. Perhaps something like this

- arg:
  - name: "model"
- let:
    name: "some_string"
    value:
      join-string:
      delimiter: "."
      children:
        - "robot"
        - "$(var model)"
        - "urdf"
        - "xacro"

could work

@christophebedard
Copy link
Copy Markdown
Member

Substitutions in frontends look like $(<name> <args>): $(var ...) is the LaunchConfiguration substitution for frontends, see

@expose_substitution('var')
class LaunchConfiguration(Substitution):

So for PathJoinSubstitution, it would be $(path-join ...) and look like this:

launch:
- let:
    name: 'model'
    value: 'a'
- log:
    message: '$(path-join robot $(var model) urdf xacro)'

This would log robot/a/urdf/xacro. Everything after path-join in $() is provided to the *Substitution class' parse() function as a sequence, see the data param here for example:

def parse(cls, data: Sequence[SomeSubstitutionsType]):
It gets a bit weird if there are spaces, but it works if you quote the item that has spaces: you can do $(path-join robot $(var model) "ur df" xacro) which results in robot/a/ur df/xacro. You can also nest substitutions, which is kind of the point of #838 (but for Python). In frontends, it looks like:

launch:
- let:
    name: 'model'
    value: 'a'
- let:
    name: 'id'
    value: '2'
- log:
    message: '$(path-join "robot$(var id)" $(var model) "ur df" xacro)'

which outputs robot2/a/ur df/xacro. I think having the correct/platform-specific path separator might not actually matter in launch files, because the underlying Python code probably translates it into the correct path string, but it might still be worth supporting. I'll open a PR to add this to PathJoinSubstitution once #838 is merged.

For StringJoinSubstitution, the first element in $(str-join ...) would have to be the delimiter, then the rest would be the components to join. This can be added in a follow-up PR, though. Once delimiter supports substitutions, I think this PR is good to go.

Signed-off-by: Christian Ruf <c.ruf99@gmail.com>
Signed-off-by: Christian Ruf <c.ruf99@gmail.com>
Signed-off-by: Christian Ruf <c.ruf99@gmail.com>
Copy link
Copy Markdown
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.

Just a small suggestion, otherwise this looks good to me!

@baconbrot
Copy link
Copy Markdown
Contributor Author

@christophebedard Alright, thanks for your review! :D

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard
Copy link
Copy Markdown
Member

Pulls: #843
Gist: https://gist.githubusercontent.com/christophebedard/5aae799688178d79a6898bdefa08f849/raw/224dcbb05e9fa97b38d47aa82b744b2d642e36cf/ros2.repos
BUILD args: --packages-above-and-dependencies launch
TEST args: --packages-above launch
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15605

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@christophebedard
Copy link
Copy Markdown
Member

The test failures seem to be unrelated.

@christophebedard christophebedard merged commit 983ec29 into ros2:rolling Apr 6, 2025
3 checks passed
@christophebedard
Copy link
Copy Markdown
Member

Would you be willing to add support for frontends, i.e., by implementing parse()? Otherwise, that's fine, I can open an issue and someone can eventually get to it.

@baconbrot
Copy link
Copy Markdown
Contributor Author

Yes! I can do that

@christophebedard
Copy link
Copy Markdown
Member

Great!

@baconbrot baconbrot deleted the stringjoinsubstitution branch April 12, 2025 09:48
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