search

ros2 / message_filters

BSD 3-Clause "New" or "Revised" License
76 stars 66 forks source link

Deprecated qos_profile in Subscriber #127

Closed CursedRock17 closed 5 months ago

CursedRock17 commented 6 months ago

This pull request is meant to solve this TODO:

// TODO(wjwwood): deprecate in favor of API's that use rclcpp::QoS instead.

In which we use rclcpp::QoS in all of the subscribe methods in message_filters. Unfortunately when it comes to deprecating functions with default arguments, we can't just change the types of one element, outlined here:

A redeclaration cannot introduce a default argument for a parameter for which a default argument is already visible (even if the value is the same)

So for this, I deprecated all the original methods, then just left the new qos argument uninitialized. I don't think this is necessarily the worst way to handle these changes because often times the user is going to need to adjust their qos settings anyways. Also, when we go to remove the deprecation in (I guess) L-turtle, we can remove the old, deprecated functions, and re-add default values if it fits our needs. Though I don't think we can leave these methods with the same names and similar default params, I'm open to suggestion on how to improve the situation.

CursedRock17 commented 6 months ago

Alright, Pull requests have been opened for those two downstream warning messages.

ahcorde commented 6 months ago
CursedRock17 commented 6 months ago

I updated the one Subscriber I was missing and also fixed up the docs so they wouldn't be deprecated.

ahcorde commented 6 months ago
CursedRock17 commented 6 months ago

That's another package downstream prepped for patch, that being said, without a windows machine, is there any way for me to check which builds will fail ahead of time apart from cloning all the repos we use downstream then grepping for certain methods which have been chaned. That way in the future I can do this without having to restart CI for y'all everytime.

ahcorde commented 6 months ago