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.
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.
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.
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.
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.
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.
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
mikaelarguedas
force-pushed
the
melodic_deprecate_headers
branch
from
cac42ff
to
42f99df
Compare
No description provided.