-
Notifications
You must be signed in to change notification settings - Fork 310
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
[BREAKING CHANGE] Use robot_description
topic instead of ~/robot_description
and update docs regarding this
#1410
Conversation
Nowadays the parameter is deprecated and will get removed.
Using robot_description by default instead will work for most people out of the box while also working correctly with different descriptions in different namespaces when using remaps.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1410 +/- ##
===========================================
+ Coverage 0 75.88% +75.88%
===========================================
Files 0 41 +41
Lines 0 3359 +3359
Branches 0 1935 +1935
===========================================
+ Hits 0 2549 +2549
- Misses 0 418 +418
- Partials 0 392 +392
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, and thanks for updating docs :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the documentation!
I've added some minor suggestions. What do you think about them?
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
@fmauch Can you fix the format locally and push?, apart from that everything looks good to me |
Yes, but not before tonight |
Found a short slot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please backport to humble |
Unfortunately, we cannot backport this to the Humble LTS distro, it would break lots of existing setups. |
Since now the desired way of receiving the
robot_description
is the topic rather than the parameter, we need to update the documentation accordingly as mentioned in #1045 (comment).As discussed in #1358 (comment) and #1138 I've used the
robot_description
topic as this is where we want it to be rather than~/robot_description
.To make this correct, I've also added a commit changing the subscription from
~/robot_description
torobot_description
which is why this is a behavior breaking change. If we merge this here, we then can update #1358 accordingly (I can make a separate PR to that branch, if you like @destogl)