-
Notifications
You must be signed in to change notification settings - Fork 95
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 .h headers in favor of .hpp headers #81
Conversation
|
||
} // namespace class_loader | ||
// *INDENT-OFF* (prevent uncrustify from adding indention below) | ||
#warning Including header <class_loader/class_loader.h> is deprecated, \ |
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 am not sure if this justifies a deprecation warning. Why can't existing code just continue to use the .h
file without the need to change?
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.
The goal here is to remove these headers in a future version of ROS. In this version the users will be able to keep using the .h headers. The goal of the deprecation warning is to warn users to update their include statements to the new headers before they get removed (in the next ROS version). That's what we've been doing for pluginlib as well: ros/pluginlib#70.
The motivation for changing the headers is to allow tools (like linters, IDE etc) to process them properly and to be more semantically correct. The new header layout will be backported to previous distributions and tickets will be filed on downstream packages to ease transition.
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 just want to point out that you are "forcing" potentially a lot of maintainers to update their code for no "good" reason. I you are planning to make PR for each affected downstream package that would be good.
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.
Just keep in mind that there might be more packages out there which might not be released or even public which all need to be updated.
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 you are planning to make PR for each affected downstream package that would be good.
Yes that's the plan. I'm considering providing a script to convert as well for non public packages (ala https://github.com/ros/pluginlib/blob/kinetic-devel/scripts/plugin_macro_update).
Just keep in mind that there might be more packages out there which might not be released or even public which all need to be updated.
Indeed, the "good" reason being: moving toward ROS1 / ROS2 compatibility. Arguably that's not a "good enough" reason, I'm mostly looking for a way to give users incentive to move forward without breaking their code. Apparently just mentioning it in the headers and documentation has not been enough in the past (in the case of pluginlib I found more than 70 packages still using the deprecated macros after several years).
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 conclusion I'll move forward with this with a few extra items:
- backport the new headers to all active ros distributions
- open PR on all downstream packages to use the new headers (once they're released on all distros)
- a script to perform the conversion automatically
- note about the new headers in the melodic migration guide
cac42ff
to
42f99df
Compare
No description provided.