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

[SW-1624] Deprecate old wrapper name #136

Merged

Conversation

tcappellari-bdai
Copy link
Collaborator

Proposed changes

Deprecate the old name bdai_ros2_wrappers for new name synchros2

test_import.py still passes and prints expected deprecation message

Checklist

  • Lint and unit tests pass locally
  • I have added tests that prove my changes are effective
  • I have added necessary documentation to communicate the changes

Additional comments

@tcappellari-bdai tcappellari-bdai changed the title [SW-1624] Deprecate old wrapper name Draft: [SW-1624] Deprecate old wrapper name Dec 20, 2024
@tcappellari-bdai tcappellari-bdai marked this pull request as draft December 20, 2024 17:17
@tcappellari-bdai tcappellari-bdai marked this pull request as ready for review December 20, 2024 20:11
@tcappellari-bdai tcappellari-bdai changed the title Draft: [SW-1624] Deprecate old wrapper name [SW-1624] Deprecate old wrapper name Dec 20, 2024
@tcappellari-bdai tcappellari-bdai requested review from khughes-bdai and a team December 20, 2024 20:20
@jcarpinelli-bdai
Copy link

All looks good to me. Just so I understand, what is the behavior that this PR changes? This basically allows someone to type import bdai_ros2_wrappers and get a warning telling them to import synchros2 instead?

If that's true, and nothing is really (substantive) is really imported with import bdai_ros2_wrappers, I wonder if this could / should be an exception or error instead. Something like...

raise ModuleNotFoundError(
"""
...renamed to synchros2...
""")

@tcappellari-bdai
Copy link
Collaborator Author

All looks good to me. Just so I understand, what is the behavior that this PR changes? This basically allows someone to type import bdai_ros2_wrappers and get a warning telling them to import synchros2 instead?

If that's true, and nothing is really (substantive) is really imported with import bdai_ros2_wrappers, I wonder if this could / should be an exception or error instead. Something like...

raise ModuleNotFoundError(
"""
...renamed to synchros2...
""")

I don't want to break anything thats already depending on bdai_ros2_wrappers just yet. Currently, importing it will actually import synchros2 so the warning is a reminder to change it. We'll officially deprecate and put in the module not found error later

@tcappellari-bdai tcappellari-bdai merged commit ef5ee62 into main Jan 2, 2025
4 checks passed
@tcappellari-bdai tcappellari-bdai deleted the SW-1624-deprecate-older-wrappers-in-ros-utilities branch January 2, 2025 18:43
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.

3 participants