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

Use context manager form of rclpy.init() #4813

Merged
merged 2 commits into from
Oct 16, 2024
Merged

Use context manager form of rclpy.init() #4813

merged 2 commits into from
Oct 16, 2024

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Oct 15, 2024

Rolling-specific code for #4780

@sloretz sloretz mentioned this pull request Oct 15, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 15, 2024 •
edited
Loading

HTML artifacts: https://github.com/ros2/ros2_documentation/actions/runs/11356701015/artifacts/2060999158.

To view the resulting site:

  1. Click on the above link to download the artifacts archive
  2. Extract it
  3. Open html-artifacts-4813/index.html in your favorite browser

@sloretz sloretz force-pushed the sloretz_ros1migration_rclpy_context_manager branch from 79009b7 to a50734d Compare October 15, 2024 02:12
fujitatomoya
fujitatomoya approved these changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@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.

lgtm. all the other places use this manner in rolling.

(i think this is draft because dependent PR is not merged yet, but lgtm anyway.)

clalancette
clalancette approved these changes Oct 15, 2024
View reviewed changes
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.

Looks good once we merge in #4780

Base automatically changed from sloretz__ROS-1-to-2_python_tutorial to rolling October 15, 2024 20:43
@clalancette
Copy link
Contributor

THis one needs a merge/rebase, then we can merge it in.

@sloretz
Use context manager form of rclpy.init()
cf37f1d 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Fix indentation
b352081 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz sloretz force-pushed the sloretz_ros1migration_rclpy_context_manager branch from a50734d to b352081 Compare October 16, 2024 00:40
@sloretz sloretz marked this pull request as ready for review October 16, 2024 00:40
@sloretz sloretz requested a review from audrow as a code owner October 16, 2024 00:40
@sloretz sloretz merged commit b5b8578 into rolling Oct 16, 2024
4 checks passed
@sloretz sloretz deleted the sloretz_ros1migration_rclpy_context_manager branch October 16, 2024 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@clalancette clalancette clalancette approved these changes

@audrow audrow Awaiting requested review from audrow audrow is a code owner

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

Successfully merging this pull request may close these issues.
3 participants
@sloretz @clalancette @fujitatomoya