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.

Sign up for GitHub

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

Jump to bottom

Replace deprecated spin_until_future_complete with spin_until_complete #306

Closed
wants to merge 2 commits into from
Closed

Replace deprecated spin_until_future_complete with spin_until_complete #306

wants to merge 2 commits into from

Conversation

hliberacki
Copy link

Signed-off-by: Hubert Liberacki [email protected]

@hliberacki
Copy link
Author

Due to change in RCLCPP - ros2/rclcpp#1874 Pull request

fujitatomoya
fujitatomoya requested changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

Can we avoid unnecessary changes to re-format?

@hliberacki
Copy link
Author

@fujitatomoya i will restore all formating base changes, they were due to some formatting plugin in IDE which did it implicitly (i did not check the diff good enough). it was not my intention

jacobperron
jacobperron reviewed Apr 1, 2022
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

For context, this connects to ros2/rclpy#919

launch_testing_ros/test/examples/set_param_launch_test.py Outdated Show resolved Hide resolved
launch_testing_ros/test/examples/set_param_launch_test.py Outdated Show resolved Hide resolved
@hliberacki hliberacki force-pushed the hliberacki/spin_until branch from 89c87b8 to d65ad3e Compare April 2, 2022 21:28
hidmic
hidmic approved these changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM with passing CI!

@hidmic
Copy link
Contributor

hidmic commented Apr 4, 2022

@ros-pull-request-builder retest this please

@hliberacki
Copy link
Author

LGTM with passing CI

Unfortunately, this CI will most probably fail. This is because it did not include all the related PRs and there will be a missing include issue. Here is a CI run for all my changes in each connected PRs.

ros2/rclcpp#1874 (comment)

@hidmic
Copy link
Contributor

hidmic commented Apr 4, 2022

@hliberacki thanks for the heads up.

Indeed, CI will fail w/o ros2/rclpy#919.

@clalancette clalancette mentioned this pull request Jun 24, 2022
Merged
@wjwwood
Copy link
Member

wjwwood commented Jun 24, 2022

Since this is part of rclpy changes, it's blocked on ros2/rclpy#919 (review), but is not needed to fix CI after ros2/rclcpp#1874 was merged prematurely.

@fujitatomoya fujitatomoya mentioned this pull request Jun 24, 2022
Closed
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:19
@fujitatomoya fujitatomoya mentioned this pull request Jan 23, 2023
Open
@christophebedard christophebedard mentioned this pull request Apr 2, 2024
Closed
@christophebedard
Copy link
Member

I've opened #396 to replace this PR.

@christophebedard
Copy link
Member

This can be closed, see #396 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobperron jacobperron jacobperron left review comments

@fujitatomoya fujitatomoya fujitatomoya requested changes

@SteveMacenski SteveMacenski SteveMacenski approved these changes

@hidmic hidmic hidmic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@hliberacki @hidmic @wjwwood @christophebedard @jacobperron @SteveMacenski @fujitatomoya @clalancette