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

Add pathlib.Path to SomeSubstitutionsType #537

Closed
wants to merge 1 commit into from

Conversation

DLu
Copy link
Contributor

@DLu DLu commented Oct 22, 2021

I'm making this change relatively blindly as a way to start discussion.

In ament/ament_index#73 I added the ability to load the path to a package as a pathlib.Path. I'd like to be able to directly use that in my launch files without manually converting to str. Consider the following example:

from ament_index_python.packages import get_package_share_path

from launch import LaunchDescription
from launch.actions import IncludeLaunchDescription
from launch.launch_description_sources import PythonLaunchDescriptionSource

def generate_launch_description():
    pkg_path = get_package_share_path('super_launch_package')
    launch_py_1 = PythonLaunchDescriptionSource(str(pkg_path / 'launch/launch_1.launch.py'))
    launch_py_2 = PythonLaunchDescriptionSource(str(pkg_path / 'launch/launch_2.launch.py'))
    ld.add_action(IncludeLaunchDescription(launch_py_1))
    ld.add_action(IncludeLaunchDescription(launch_py_2))
    return ld

Questions I have:

  • Where are there repercussions of this that I'm missing?
  • Where should tests be added?

Signed-off-by: David V. Lu <davidvlu@gmail.com>
@hidmic
Copy link
Contributor

hidmic commented Oct 27, 2021

@DLu I see the convenience and simplicity of supporting implicit str coercions, but I wonder if we should open the door to (even) more opaque launch descriptions to spare code in some actions' __init__. I.e. PythonLaunchDescriptionSource could take a substitution or a path, and that'd yield the same UX.

@DLu
Copy link
Contributor Author

DLu commented Oct 29, 2021

@hidmic The other place where I want to use it is in specifying parameter files to load, a la

parameters = [[pkg_path, 'config', 'ompl_planning.yaml']]

@hidmic
Copy link
Contributor

hidmic commented Oct 29, 2021

@DLu hmm, would it work if a pathlib.Path instance like pkg_path / 'config' / 'ompl_planning.yaml' was accepted?

@DLu
Copy link
Contributor Author

DLu commented Oct 29, 2021

@hidmic Yes, but I was also thinking about the wacky inclusion stuff that MoveIt configs sometimes do and so a pathlib might be combined with a LaunchConfiguration.

@hidmic
Copy link
Contributor

hidmic commented Oct 29, 2021

Hmm, good point. @wjwwood @ivanpauno WDYT?

@ivanpauno
Copy link
Member

Hmm, good point. @wjwwood @ivanpauno WDYT?

SomeSubstitutionsType is also used for things that are not a path, so I don't think we should modify that.

We could have add a SomePathType definition, similar to what's proposed here.
We would also need a method to resolve that SomePathType "substitution" when a context is provided.

@DLu
Copy link
Contributor Author

DLu commented Nov 11, 2021

I'm closing this for now, as calling str() is not too onerous.

@DLu DLu closed this Nov 11, 2021
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

3 participants