-
Notifications
You must be signed in to change notification settings - Fork 156
Allow include:
to take a single text element instead of requiring file:
#867
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?
Allow include:
to take a single text element instead of requiring file:
#867
Conversation
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Discussed in PMC meeting today. Fielded some agreement with the concern that it's "one more way to do things", but also no strong objection either. Relatively neutral. @tfoote brought up that we may want to support |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-pmc-meeting-minutes-for-april-29-2025/43451/1 |
@wjwwood @christophebedard @ahcorde could I get your thoughts on this one? I still think it's a good idea, but I'd like us to decide one way or the other. Either move forward, or decide that we don't want this. |
Like discussed, there is a benefit to having a really explicit data model. For YAML that data model translates to these terms (XML has the same structure but uses slightly different terms based on its syntax):
This proposal I believe boils down to changing 2b. to something like: "2b. Some Actions have a special case 'default attribute key' such that if no other attributes are needed, you can provide a value directly - which may be text, mapping, or list depending on the attribute. For example, in Which to me makes as much sense as the |
I agree that The only thing I'm not that happy about is having another way of providing the file to the include action, which you did mention. How do we avoid confusing users? Especially since no other (YAML) action currently has both a "bare text" value and child entities. I'd like to see documentation, both (1) in this PR and (2) to go along with this PR:
|
@property | ||
def bare_text(self) -> Optional[Text]: | ||
"""Return the bare text of this element if it is a bare text element, or None.""" | ||
raise NotImplementedError() |
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.
Can we find a more descriptive name for this, or just describe it using words other than "bare text" in the docstring, so that it's a bit clearer?
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, a different name is fine, i just wasn't sure what to call it exactly
Closes robograph-project/planning#15
Not sure this has to be in launch, but it's part of an ergonomics sweep I'm doing while writing more YAML/XML launchfiles.
I felt that for simple actions, it made sense to be able to just pass the single argument as a bare text element to the action, rather than requiring a
list
ordict
contents.Specifically, the following felt obvious, but was not supported, and probably makes sense for some other Actions.
Admittedly, the XML one is actually more verbose, it's a bigger improvement for simple YAML.
This is a complete enhancement PR with unit tests for the new feature, if others agree that this makes sense.
Argument for: This matches the Python API more directly since
IncludeLaunchDescription
takes a non-keyword first argument for thelaunch_description_source
. It also allows for shorter, clearer launchfiles for simple usageArgument against: most real usage has other attributes,
arg
etc, allowing this simple syntax could make things more confusing by having multiple ways of doing things.