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

Fix deprecated subscription callbacks in tutorials #2079

Merged
merged 2 commits into from
Nov 4, 2021

Conversation

aprotyas
Copy link
Member

@aprotyas aprotyas commented Oct 30, 2021

Namely, the void (shared_ptr<MsgT>) subscription callback has
been deprecated (see ros2/rclcpp#1713).

As such, snippets using that signature have been updated with the
void (shared_ptr<const MsgT>) subscription callback.

Signed-off-by: Abrar Rahman Protyasha [email protected]

@aprotyas
Copy link
Member Author

aprotyas commented Oct 30, 2021

FYI, I believe void (const MsgT&) is the best signature for "read-only" callbacks (see ros2/rclcpp#1598 (comment)), but was not sure if that's too disruptive a change in these snippets.

Copy link
Member Author

@aprotyas aprotyas left a comment

Choose a reason for hiding this comment

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

Also committed a couple of changes that were for a different deprecation in rosbag2_cpp::writer_interfaces::Writer (see ros2/rosbag2#866).

aprotyas and others added 2 commits November 4, 2021 17:03
Namely, the `void shared_ptr<MsgT>` subscription callback has been
deprecated. As such, snippets using that signature has been replaced
with `void shared_ptr<const MsgT>` subscription callbacks.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette force-pushed the aprotyas/fix_deprecated_sub_callbacks branch from 21018ea to e2e1f54 Compare November 4, 2021 21:15
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

All right, I added the fixes that make it work for me on Rolling, and I've also rebased this on top of #2095 . This looks good to me now, thanks for the update @aprotyas !

@clalancette clalancette merged commit fb5ffc1 into rolling Nov 4, 2021
@delete-merged-branch delete-merged-branch bot deleted the aprotyas/fix_deprecated_sub_callbacks branch November 4, 2021 21:19
@jdlangs
Copy link

jdlangs commented Feb 8, 2022

I would have thought there should be a clear preference to update things to the const & signatures here to make the documentation default simpler instead of more verbose. @aprotyas was there any specific reason you thought it might be "too disruptive"?

@aprotyas
Copy link
Member Author

aprotyas commented Feb 8, 2022

I would have thought there should be a clear preference to update things to the const & signatures here to make the documentation default simpler instead of more verbose. @aprotyas was there any specific reason you thought it might be "too disruptive"?

You're right. Not sure why I didn't just update to const & signatures - I reckon because I made similar changes in ros2/rclcpp#1713. I should probably also update those signatures.

Except for the rosbag tutorial, where we want to be using a shared pointer to rclcpp::SerializedMessage callback, I've updated the other read-only subscription callbacks to reflect the preferred signature (#2297).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants