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

Use singleton approach to store and reuse the service clients (backport #1949) #1953

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Dec 17, 2024

Fixes: #1934

the node.destroy_node() at the end does the same thing by destroying clients and every subscribers, publishers etc (https://github.com/ros2/rclpy/blob/8f1f16f16090184062f1993728d063ff2680f958/rclpy/rclpy/node.py#L2017-L2053)


This is an automatic backport of pull request #1949 done by Mergify.

@mergify mergify bot added the conflicts label Dec 17, 2024
Copy link
Contributor Author

mergify bot commented Dec 17, 2024

Cherry-pick of b039baa has failed:

On branch mergify/bp/humble/pr-1949
Your branch is up to date with 'origin/humble'.

You are currently cherry-picking commit b039baa.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   controller_manager/controller_manager/spawner.py

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   controller_manager/controller_manager/controller_manager_services.py

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@christophfroehlich
Copy link
Contributor

Backport fails because #1703 hasn't been backported. @saikishor could you fix that pls?

@saikishor saikishor force-pushed the mergify/bp/humble/pr-1949 branch from ce9d65f to 6dbd5d3 Compare December 17, 2024 16:25
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 62.92%. Comparing base (d7fcbca) to head (6dbd5d3).
Report is 1 commits behind head on humble.

Files with missing lines Patch % Lines
.../controller_manager/controller_manager_services.py 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           humble    #1953      +/-   ##
==========================================
+ Coverage   62.87%   62.92%   +0.05%     
==========================================
  Files         109      109              
  Lines       12494    12505      +11     
  Branches     8481     8479       -2     
==========================================
+ Hits         7855     7869      +14     
+ Misses        854      852       -2     
+ Partials     3785     3784       -1     
Flag Coverage Δ
unittests 62.92% <93.33%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
controller_manager/controller_manager/spawner.py 70.07% <100.00%> (+0.22%) ⬆️
.../controller_manager/controller_manager_services.py 82.92% <92.85%> (+1.10%) ⬆️

... and 1 file with indirect coverage changes

@christophfroehlich
Copy link
Contributor

The namespacing support "silently" is added now?

@saikishor
Copy link
Member

The namespacing support "silently" is added now?

Yes! It always existed in the name of the argument, but with this even the cli should be happy. Moreover, this only applies if the service is relative and not absolute

If you prefer, not to have it. I can fix it

@christophfroehlich christophfroehlich merged commit 619413c into humble Dec 17, 2024
13 checks passed
@christophfroehlich christophfroehlich deleted the mergify/bp/humble/pr-1949 branch December 17, 2024 18:41
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.

2 participants