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

Python Version of Angles.h #7

Merged
merged 3 commits into from
May 13, 2016
Merged

Python Version of Angles.h #7

merged 3 commits into from
May 13, 2016

Conversation

DLu
Copy link
Collaborator

@DLu DLu commented Sep 28, 2015

It's always bothered me that this doesn't exist.

I'm not entirely certain I did the test setup correct, but it does pass when run manually.

#*********************************************************************
# Software License Agreement (BSD License)
#
# Copyright (c) 2008, Willow Garage, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copied from here: https://github.com/ros/angles/blob/master/angles/include/angles/angles.h

What is the policy for updating Willow Garage copyrights? Nav stack is full of them.

Copy link
Member

Choose a reason for hiding this comment

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

You wrote the new code. You get the copyright (though if you're working for a company you may be required to give it to them.). And it should be dated the year written.

When you significantly edit a file, you should add the name and year at the top to a list in the header.

@DLu
Copy link
Collaborator Author

DLu commented May 13, 2016

Figuring out the license was a longer process than I thought.

Ready for review again.


return 2*pi

def find_min_max_delta(from_angle, left_limit, right_limit):
Copy link
Member

Choose a reason for hiding this comment

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

This would be great to prefix with an underscore to signify that it's not public API.

@tfoote
Copy link
Member

tfoote commented May 13, 2016

Thanks for resolving the license. One comment on the function name but otherwise lgtm. Especially thanks for all the unit tests.

@tfoote tfoote merged commit 4f1676b into ros:master May 13, 2016
@tfoote
Copy link
Member

tfoote commented May 13, 2016

Sorry, I just got the PR build test working and forgot about the one change requested. Could you tweak that an open a new PR?

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