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

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

Closed
Interpause opened this issue Nov 2, 2024 · 1 comment

Comments

@Interpause
Copy link

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
Copy link
Author

Interpause commented Nov 2, 2024

is solved by #790 in a more explicit way

@Interpause Interpause closed this as not planned Won't fix, can't repro, duplicate, stale Nov 2, 2024
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 a pull request may close this issue.

1 participant