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

Deprecate LOCAL2 in SphericalCoordinates #451

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Aug 22, 2024

🦟 Bug fix

This PR extends PR #450 .

Summary

In this PR, I include #450 to fix all remaining build failures.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

arjo129 and others added 2 commits August 19, 2024 15:32
With the introduction of gazebosim/gz-math#616.
We will be deprecating `LOCAL2` as a work around in favor or `LOCAL`. As
part of this change, its probably a good idea to update `gz-msgs` as
well.

Signed-off-by: Arjo Chakravarty <[email protected]>
@peci1 peci1 requested a review from caguero as a code owner August 22, 2024 01:55
@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Aug 22, 2024
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this. I think we should add a note in the migration file.


GZ_UTILS_WARN_IGNORE__DEPRECATED_DECLARATION
Copy link
Contributor

Choose a reason for hiding this comment

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

This works great!

I was trying to understand how this prevents warnings in gz-msgs, but still gives warnings in downstream libraries when you try to use the LOCAL2 enum, which is what we want. The warning this prevents is the warning emitted by just including msgs/details/spherical_coordinates.pb.h because that file contains

constexpr SphericalCoordinatesType SphericalCoordinatesType_MAX = LOCAL2;

which is a direct use of the LOCAL2 enum.

when downstream libraries include msgs/spherical_coordinates.pb.h, they won't get a warning from that. But if they use LOCAL2 directly, a warning is emitted.

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey
Copy link
Contributor

azeey commented Aug 22, 2024

I've added a migration note in 338b2b8. It would be great if, in a follow-up PR, you can modify it to add more detail and replace the PR link I added. In the mean time, I'll merge this to fix CI on gz-transport.

@azeey azeey merged commit e99b705 into gazebosim:main Aug 22, 2024
7 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants