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

RosTimer -> ROSTimer and PushRosNamespace -> PushROSNamespace, to follow PEP8 #326

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Sep 24, 2022

Follow up of #325 (comment)

I didn't actually see these being used anywhere else in the standard ROS repositories. But I did find a few places in the documentation: ros2/ros2_documentation#3038

…low PEP8

Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
Copy link
Member Author

wjwwood commented Sep 24, 2022

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we deprecate the old names to make the transition easier?
Do we have any linter complaining about this?

@wjwwood
Copy link
Member Author

wjwwood commented Sep 27, 2022

I was going to deprecate in a separate pr, since we could back port this one, but probably only want to deprecate in rolling. I just haven't had time.

@wjwwood
Copy link
Member Author

wjwwood commented Sep 27, 2022

Do we have any linter complaining about this?

If you're asking about the way the class is named, it's not possible for a linter to know one way or the other... I guess you could say something specific like *Ros[A-Z].* should be ROS instead. But no we don't have that.

@clalancette
Copy link
Contributor

I was going to deprecate in a separate pr, since we could back port this one, but probably only want to deprecate in rolling. I just haven't had time.

Wait, I'm confused. If we were to backport this, wouldn't we be breaking compatibility in old distributions? That is, anything that called PushRosNamespace before would now break, right?

@wjwwood
Copy link
Member Author

wjwwood commented Sep 27, 2022

This pr doesn't break API, it just adds new API (well rename and add an alias to the old name). No downstream code is required to change yet.

@wjwwood
Copy link
Member Author

wjwwood commented Sep 27, 2022

I was thinking we could back port it as a convenience for downstream users that want to use the new class name in all branches (or have a single branch for Humble and Rolling and more).

@ivanpauno
Copy link
Member

This pr doesn't break API, it just adds new API (well rename and add an alias to the old name). No downstream code is required to change yet.

Sorry, I didn't see you added an alias when reviewing before.
LGTM!

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 this pull request may close these issues.

4 participants