Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@christophebedard meta: I wonder if it'd make sense to change
expose_action
to automatically register "underscore" and "dash" aliases. CC @ivanpauno @wjwwood.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.
Even if we did this (just this pr or the automatic mapping) I really think we should pick one way and stick with it.
Ideally there wouldn't be another way to do it.
I'm slightly 👎 to changing the documentation to use underscores after merging this, if we use dashes everywhere else.
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.
underscores are used everywhere else. This one action is the only one that uses dashes (https://github.com/ros2/ros2_documentation/pull/539/files#r388486312)
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.
In that case I'd say 👍 for this change and doc change but less so for the automatic mapping. Again preferring one good way to do things.
I'd even be in favor of deprecating the use of dashes.
But I could be convinced otherwise on each of those points I think.
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.
picking one and sticking with it does make sense, as does deprecating the current name with the dashes (although I'm not sure where in
launch_ros
I'd put a warning message for it).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's fair. IIRC aliases had this purely cosmetic purpose back when we first thought about this problem. Specifically, underscores are somewhat less aesthetically pleasing in XML than dashes. Accepting both does not hurt format translation. But we can be strict too.
We do not enforce one tag per entity though. Maybe we should.
There's no support for deprecation, but it's easy enough to add it. If we want to be strict, we can add a list of deprecated aliases to
expose_*
decorators. We can register the same parsing function for those aliases but decorated to emit a deprecation warning.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.
Sounds like deprecating isn't worth it. We can just change the documentation, which is probably enough.
I actually don't mind if there is the ability to use dash or underscore automatically that much, but I do think we should be consistent in the documentation and avoid dashes some place and underscores in others.
Either way sounds like this pr is good and the doc changes are a good idea too.
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'm tempted to just wipe it. Specially if it's just one. But I will say that, eventually, we'll need tick-tock deprecation cycles here too.
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.
Oh maybe I wasn't clear when I said deprecation wasn't worth it. I was thinking we'd just leave it but document that you should use underscores. So never remove it.
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.
Ooh. Yeah. I misunderstood you. I'm slightly inclined towards enforcing rather than documenting. None of this documentation is available online just yet. Until then, most people won't read it.
It doesn't have to happen in this PR though.