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

File naming conventions compliance in drake/solvers #3010

Merged
merged 13 commits into from
Aug 1, 2016

Conversation

ggould-tri
Copy link
Contributor

@ggould-tri ggould-tri commented Aug 1, 2016

drake/solvers was one of our major reservoirs of noncompliant file naming. This PR stomps on it, correcting the naming of every non-mex cpp file in the solvers directory and its test subdirectory (but not its other subdirs).

This PR is very large, but all of the changes were done by script. Once it passes CI it is likely correct, so review should focus on whether it was a good idea in the first place.

Marking do-not-review until CI passes, as this has major configuration-specific changes.


This change is Reviewable

@ggould-tri
Copy link
Contributor Author

+@liangfok feature
+@sherm1 platform

Last big automated move/rename PR for at least the next month, I promise!


Review status: 0 of 51 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@liangfok
Copy link
Contributor

liangfok commented Aug 1, 2016

:lgtm: Just one question.


Reviewed 78 of 78 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


drake/solvers/mosek_solver.h, line 1 [r1] (raw file):

// Copyright 2016, Alex Dunyak

BTW. I just noticed that these Mosek files are copyrighted by Alex Dunyak. Are we sure it's OK to modify them? Should changing the files result in any change in the copyright notice?


Comments from Reviewable

@ggould-tri
Copy link
Contributor Author

Review status: all files reviewed at latest revision, all discussions resolved.


drake/solvers/mosek_solver.h, line 1 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW. I just noticed that these Mosek files are copyrighted by Alex Dunyak. Are we sure it's OK to modify them? Should changing the files result in any change in the copyright notice?

I believe that with the `LICENSE.txt` we have, the copyright header is functionally irrelevant. But it certainly is inconsistent.

This is not an issue for this PR, but I will put header rectification on my to-do list.


Comments from Reviewable

@liangfok
Copy link
Contributor

liangfok commented Aug 1, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


drake/solvers/mosek_solver.h, line 1 [r1] (raw file):

Previously, ggould-tri wrote…

I believe that with the LICENSE.txt we have, the copyright header is functionally irrelevant. But it certainly is inconsistent.

This is not an issue for this PR, but I will put header rectification on my to-do list.

SGTM!

Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Aug 1, 2016

Platform review: yes, good idea; CI worked; one minor change needed and a few BTWs for your consideration. :lgtm:


Reviewed 78 of 78 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


CHANGELOG.md, line 48 [r1] (raw file):

 - [#2779][] Move some rotation functions from drakeGeometryUtil to drake/math.
 - [#2963][] Rename RigidBody::CollisionElement to RigidBodyCollisionElement.
 - [#3010][] All header file names under `solvers` are now spelled with lower case and underscore names.

You have to add the actual URL at the bottom of this file.


CHANGELOG.md, line 49 [r1] (raw file):

 - [#2963][] Rename RigidBody::CollisionElement to RigidBodyCollisionElement.
 - [#3010][] All header file names under `solvers` are now spelled with lower case and underscore names.

BTW, the ordering within the changelog seems random. I think it was reverse-chronological for a while but then a few changes got tacked on to the end. I suggest moving yours and the other late-bloomers to the top.


drake/solvers/mosek_solver.h, line 1 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

SGTM!

BTW, since the code doesn't refer to the LICENSE.txt file, this is currently saying the code is non-open source and can only be used with Alex's permission. We should ask him to agree to license under Drake's terms or remove the code from Drake.

drake/solvers/qp.cc, line 1 [r1] (raw file):

#include <math.h>

BTW, should be <cmath>.


Comments from Reviewable

@ggould-tri ggould-tri force-pushed the flag_day_solver_renames branch from 17f636a to 306e085 Compare August 1, 2016 19:47
@ggould-tri
Copy link
Contributor Author

Review status: 43 of 51 files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 48 [r1] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

You have to add the actual URL at the bottom of this file.

Done.

CHANGELOG.md, line 49 [r1] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW, the ordering within the changelog seems random. I think it was reverse-chronological for a while but then a few changes got tacked on to the end. I suggest moving yours and the other late-bloomers to the top.

Moved mine and one other I know to be recent (because I reviewed it).

drake/solvers/mosek_solver.h, line 1 [r1] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW, since the code doesn't refer to the LICENSE.txt file, this is currently saying the code is non-open source and can only be used with Alex's permission. We should ask him to agree to license under Drake's terms or remove the code from Drake.

I am relaying this to Russ, as Alex is one of his.

Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Aug 1, 2016

:lgtm: -- all good.


Reviewed 3 of 78 files at r1, 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@ggould-tri ggould-tri force-pushed the flag_day_solver_renames branch from 306e085 to d7e8fe8 Compare August 1, 2016 20:25
@liangfok
Copy link
Contributor

liangfok commented Aug 1, 2016

:lgtm:


Reviewed 4 of 78 files at r1, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@ggould-tri ggould-tri merged commit 01f9ddc into RobotLocomotion:master Aug 1, 2016
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