-
Notifications
You must be signed in to change notification settings - Fork 158
Add FindLaunchfile substitution #915
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
base: rolling
Are you sure you want to change the base?
Conversation
…h valid suffixes in a directory Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
| if len(data) != 2: | ||
| raise AttributeError( | ||
| f'find-launchfile substitution expects 2 argument, {len(data)} given') | ||
| return cls, {'name': data[0], 'path': data[1]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make more sense to make the arguments $(find-launchfile PATH FILE) instead of FILE PATH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open for debate, my reasoning was "find name in dir" which matches "grep pattern in location".
I'm sure a sensible argument could be made for the opposite as well. But I'll stick with this implementation as my suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess it is a matter of opinion.
An argument for my alternative would be that it would match the ros2launch argument ordering (in the package case), which makes the most sense for the suggested IncludePackageLaunch.
Keeping the same order of [package/folder, launchfile'] for this substitution would keep everything consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is sensible, yes. Internal consistency is a good argument 👍 I'll change it
| def name_matches_launchfile(name: Text, file: Path) -> bool: | ||
| from ..frontend import Parser # avoid circular import | ||
| valid_extensions = {'py', *Parser.get_available_extensions()} | ||
| valid_suffixes = {'', '.launch', '_launch'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't allowing a non-suffix lead to matching Python package files as well?
I could imagine a package containing my_node.py and my_node.launch.py, which might introduce issues in the IncludePackageLaunch case.
|
@emersonknapp, have you considered what to do with subfolders? e.g: Should it be able to find the |
|
@SuperJappie08 I thought about it but didn't come to a conclusion. Maybe it should take a recursive boolean argument? |
|
I think it really depends on the way this substitution is framed:
If recursion were to be implemented, I can think of 3 options:
But after writing this, I feel the best option could just be to rename it to |
Description
Fixes #849
FindLaunchfilesubstitution can find launch files in a directory by stem name only, if it matches valid suffixes.This enables users to not care what language an included launchfile is written in, or whether it uses
.launchor_launchstyle.So instead of
the user can do:
This is slightly more verbose, but incredibly more flexible.
Now if the package maintainer migrates their Python launchfile to YAML, for example - we as users don't have to change our launchfiles and we'll automatically include the new one.
This change enables new features in
launch_rosthat I'll push soon:FindPackageLaunch(pkg, name)andIncludePackageLaunch(pkg, name). These will utilizeFindPackageShareDirectory+FindLaunchfilein combination.CAVEAT: if maintainers follow the "redirect migration strategy" that I recommend in my upcoming roscon talk (below), there will now be two launchfiles matching the searched stem, which will fail the substitution. But I'm not sure what the strategy should be to mitigate that.
I can think of these possible strategies:
FindLaunchfile(name='stem', path=launch_dir, prefer='yaml'). If there's only python, it will return that. If py and yaml, then the yaml one returned. If py and xml, then... error? This is probably too confusing of an idea.Probably the "you should only have one launchfile per stem" soft constraint is better.
Is this user-facing behavior change?
New feature!
Did you use Generative AI?
No
Additional Information
N/A