ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
127 stars 141 forks source link

[Enhancement] `normalize_to_list_of_substitutions` should cast `pathlib.Path` to `str` #805

Closed Interpause closed 8 hours ago

Interpause commented 10 hours ago

Feature request

Feature description

Currently, for IncludeLaunchDescription, I have to do this:

pkg_uwu_sim = get_package_share_path("uwu_sim")
xacro = IncludeLaunchDescription(
   str(pkg_uwu_sim / "launch" / "xacro.launch.py"),
   launch_arguments={"robot": ROBOT_TYPE}.items(),
)

It would be nicer if I could just pass in pathlib.Path without casting to string first:

xacro = IncludeLaunchDescription(
   pkg_uwu_sim / "launch" / "xacro.launch.py",
   launch_arguments={"robot": ROBOT_TYPE}.items(),
)

However, doing the above currently results in TypeError: 'PosixPath' object is not iterable. Most likely related to #345, but I'll send in a PR afterwards to fix this specific case in a way that might fix many others.

Implementation considerations

In short, always treat pathlib.Path as a str for TextSubstitution. Tracing the source code from IncludeLaunchDescription, these are the steps taken on the launch_description_source argument:

  1. In IncludeLaunchDescription, if launch_description_source isn't an instance of LaunchDescriptionSource, wrap it in AnyLaunchDescriptionSource.
  2. In AnyLaunchDescriptionSource, pass the launch_file_path to parent's __init__().
  3. In LaunchDescriptionSource, set self.__location to normalize_to_list_of_substitutions(location).

It is in normalize_to_list_of_substitutions when the error occurs, as since pathlib.Path isn't a str nor Substitution, it attempts to cast it to an Iterable to normalize each item within it, which isn't valid.

My proposal and PR later:

def normalize_to_list_of_substitutions(subs: SomeSubstitutionsType) -> List[Substitution]:
    """Return a list of Substitutions given a variety of starting inputs."""
    # Avoid recursive import
    from ..substitutions import TextSubstitution

    def normalize(x):
        if isinstance(x, Substitution):
            return x
        if isinstance(x, str):
            return TextSubstitution(text=x)
        raise TypeError(
            "Failed to normalize given item of type '{}', when only "
            "'str' or 'launch.Substitution' were expected.".format(type(x)))

    ### >>> Insert
    if isinstance(subs, Path):
        return [TextSubstitution(text=str(subs.resolve()))]
    ### <<< End
    if isinstance(subs, str):
        return [TextSubstitution(text=subs)]
    if is_a_subclass(subs, Substitution):
        return [cast(Substitution, subs)]
    return [normalize(y) for y in cast(Iterable, subs)]

Since normalize_to_list_of_substitutions is used in many places, simply casting pathlib.Path to str may fix a lot of things, where the previous behaviour was to attempt to cast it to Iterable and crash with TypeError as a result.

Interpause commented 8 hours ago

is solved by #790 in a more explicit way