-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add push_ros_namespace
alias to push-ros-namespace
#250
Add push_ros_namespace
alias to push-ros-namespace
#250
Conversation
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
It was pretty straightforward, so I just did it: ros2/ros2_documentation#1703 |
@@ -33,6 +33,7 @@ | |||
from rclpy.validate_namespace import validate_namespace | |||
|
|||
|
|||
@expose_action('push_ros_namespace') | |||
@expose_action('push-ros-namespace') |
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.
I'm slightly to changing the documentation to use underscores after merging this, if we use dashes everywhere else.
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.
Even if we did this (just this pr or the automatic mapping) I really think we should pick one way and stick with it.
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.
picking one and sticking with it does make sense, as does deprecating the current name with the dashes
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.
Sounds like deprecating isn't worth it. We can just change the documentation, which is probably enough.
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.
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.
LGTM pending green CI.
We should revisit frontend deprecation and one-tag-per-entity policy at some point in time.
Alright, this one's ready to go. |
Closes #142
Once this is approved, I will update the documentation to use the alias with the underscores (https://github.com/ros2/ros2_documentation/pull/539/files#r409568794).
Signed-off-by: Christophe Bedard bedard.christophe@gmail.com