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

cleanup #663 #724

Merged
merged 3 commits into from
Dec 18, 2017
Merged

cleanup #663 #724

merged 3 commits into from
Dec 18, 2017

Conversation

rhaschke
Copy link
Contributor

This PR provides some cleanup to the previously merged PR #663.
As a major regression, after #663, displaying of attached collision objects didn't work anymore (#723).
Besides fixing this, the present PR provides some other minor improvements.

... because it was passed by-value instead of by-reference
return value is not needed for vector versions (zero size indicates no elements)
Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

I don't see the need to break (released) API just because it's redundant.
But I do approve the change set as is.

@rhaschke @jeffreyling sorry for the simple oversight...

@rhaschke rhaschke mentioned this pull request Dec 18, 2017
@v4hn
Copy link
Contributor

v4hn commented Dec 18, 2017

Tests for the PlanningScene #667 would have helped, but nobody got around to review this yet.
Not to mention adding a test for this case.

I'll merge this to fix the bug and remove the release blocker.

@v4hn v4hn merged commit 9b303e4 into kinetic-devel Dec 18, 2017
@rhaschke
Copy link
Contributor Author

@velveteenrobot As we had a regression after accepting your original PR #663, may I ask you to provide some unittests that validate the API to interact with the planning scene? This will help to avoid regression issues in future.
For example, you could insert collision objects through various API calls and validate that your new getters will fetch the correct messages. Thanks a lot in advance.

@velveteenrobot
Copy link
Contributor

I probably won't get around to this for like a month but if no one beats me to it, I should be able to put in some unit tests in January.

@rhaschke
Copy link
Contributor Author

Thanks. That would be great.

@130s 130s deleted the pr-cleanup-#663 branch December 25, 2017 16:46
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