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

Simplify controller manager namespacing #2210

Merged
merged 2 commits into from
Jun 5, 2023

Conversation

stephanie-eng
Copy link
Contributor

Description

This PR attempts to simplify the logic with setting a namespace (stored in the ns_ variable) for Ros2Control Manager.
My understanding of the original logic:

  • ns_ can be set via constructor, but its value is ignored and replaced by the parameter value.
  • If that parameter doesn't exist, we default the parameter to "/" and also set ns_ to match.
  • In the case where ns_ is not set via constructor, we try to get it from a parameter.
  • If that parameter doesn't exist, ns_ remains an empty string.

It sounds like the desired behaviour would be to set ns_ to the value of the ros_control_namespace parameter, if it exists, or otherwise default to "/", without considering the original value of ns_.

This PR also adds a deprecation notice to setting ns_ via constructor, since this value is ignored anyway. I've deprecated it instead of removing it outright to be safe, but I'm not sure that this is used anywhere, given that this is used as a plugin and thus should be default constructed.

@stephanie-eng stephanie-eng requested a review from sjahr May 30, 2023 14:04
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (b98bb6b) 50.53% compared to head (2dd1d73) 50.51%.

❗ Current head 2dd1d73 differs from pull request most recent head 0ee7934. Consider uploading reports for the commit 0ee7934 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2210      +/-   ##
==========================================
- Coverage   50.53%   50.51%   -0.01%     
==========================================
  Files         386      386              
  Lines       31736    31736              
==========================================
- Hits        16034    16029       -5     
- Misses      15702    15707       +5     

see 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Regardless of whether the logic is equivalent, I agree that we should stick to a single way of specifying this parameter.

@sjahr sjahr enabled auto-merge (squash) June 5, 2023 12:16
@sjahr sjahr merged commit 55df0bc into moveit:main Jun 5, 2023
henningkayser added a commit to henningkayser/moveit2 that referenced this pull request Jun 7, 2024
This reverts commit 55df0bc.

The deprecated constructor was being used in the same file
for the exact use case of enabling namespaces that are not
specified by the parameter. There is no replacement for
supporting a dynamic server lookup, however the parameter
logic could still use simplification.
henningkayser added a commit that referenced this pull request Jun 13, 2024
This reverts commit 55df0bc.

The deprecated constructor was being used in the same file
for the exact use case of enabling namespaces that are not
specified by the parameter. There is no replacement for
supporting a dynamic server lookup, however the parameter
logic could still use simplification.
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