-
Notifications
You must be signed in to change notification settings - Fork 197
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
Change TF2Error names to be a bit more descriptive. #349
Conversation
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.
You could have a message with the deprecation, like "use TF2_X instead", but otherwise lgtm.
Yeah, good point. I added that in d711136 . I'll run CI now: |
The Windows warnings are known, and are being addressed elsewhere. The macOS warnings are annoying. They are saying that One way out of this conundrum is to wait an see what happens with ros-infrastructure/rep#291 . If we decide to switch to C++17, then we can just do that and use the deprecation as-is. The other thing we can possibly do is to deprecate the enum entirely, and come up with a new enum with new names. I'm going to hold off on this one for now. |
Oh yeah, that came up in the past I think. |
d711136
to
491c704
Compare
And of course now Windows is annoyed at C++17 changes. Sigh. OK, this is (still) a puzzle for another day. |
491c704
to
256becc
Compare
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, this may generate some deprecation warnings on some other packages which it would be great to track. I think we should run CI with --packages-above-and-dependencies tf2
The driving reason behind this is to avoid the NO_ERROR macro on Windows. We can't totally get around the problem right now, but we can deprecate the old names and switch to new ones. After Galactic, we can remove the old names and get rid of the problem completely. Signed-off-by: Chris Lalancette <[email protected]>
256becc
to
cf3b76a
Compare
Instead of using C++17, which causes some downstream breakage, I think the simpler thing to do here is to ignore the warning that clang throws on macOS. This whole thing will eventually go away (for I-Turtle) anyway, so I think that is a reasonable enough thing to do. The latest commit does exactly that.
My earlier CI attempts show that it doesn't, at least not in the core. There may be some breakage outside of the core, but that's why it is deprecated, not removed :).
Yep, definitely. Will run new CI with the latest commit and everything above tf2. |
CI is green here, so I'm going to go ahead and merge this. Thanks for the reviews. @ahcorde If you have time, could you please review the release note I added for this at ros2/ros2_documentation#1957 ? Thanks. |
The driving reason behind this is to avoid the NO_ERROR macro
on Windows. We can't totally get around the problem right now,
but we can deprecate the old names and switch to new ones.
After Galactic, we can remove the old names and get rid of the
problem completely.
Signed-off-by: Chris Lalancette [email protected]
This obsoletes #247 (which I'll go ahead and close out).