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

Revert "Revert "adding API change for spin_until_complete"" #3334

Closed
wants to merge 1 commit into from

Conversation

audrow
Copy link
Member

@audrow audrow commented Feb 13, 2023

⚠️ This is waiting on ros2/rclcpp#1957 before being merged

Reverts #3333, which originally reverted #3328.

@SteveMacenski
Copy link
Contributor

This shouldn't be a draft. Its ready to go with the feature merges

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Feb 13, 2023

@audrow please revert this. (edit, i mean merge this PR, sorry for the confusion.)

@audrow audrow marked this pull request as ready for review February 13, 2023 22:42
Rename ``spin_until_future_complete`` to ``spin_until_complete``
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

`PR 1874 <https://github.com/ros2/rclcpp/pull/1874>`_ renames ``spin_until_future_complete`` to ``spin_until_complete`` to represent the semantics of being able to spin on values that are not exclusively futures.
Copy link
Member Author

Choose a reason for hiding this comment

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

@SteveMacenski, should we mention ros2/rclcpp#1957 here instead?

Seeing that ros2/rclcpp#1874 was merged is what made me think this was ready to merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the "revert revert" language in 1957 makes it less clear to a user. I'd think 1874 is a better link to understand the change, but I don't feel overly strong about it

@SteveMacenski
Copy link
Contributor

SteveMacenski commented Feb 14, 2023

@audrow please revert this. (edit, i mean merge this PR, sorry for the confusion.)

Now I'm confused -- so you want this merged now @fujitatomoya ?

@fujitatomoya
Copy link
Collaborator

Oh sorry, i was the one who got confused.

@christophebedard
Copy link
Member

I've opened #4252 to replace this PR. Closing.

@christophebedard christophebedard deleted the revert-3333-revert-3328-spin_until2 branch April 2, 2024 22:45
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.

4 participants