ahundt / grl

Robotics tools in C++11. Implements soft real time arm drivers for Kuka LBR iiwa plus V-REP, ROS, Constrained Optimization based planning, Hand Eye Calibration and Inverse Kinematics integration.
https://ahundt.github.io/grl/
BSD 2-Clause "Simplified" License
153 stars 72 forks source link

Code cleanup #134

Open ahundt opened 7 years ago

ahundt commented 7 years ago

I've had a request for code cleanup, so I wanted to lay out the steps necessary to incorporate such a change.

At this point I've generally prioritized accepting contributions over code consistency for practical reasons, plus some code (like the vrep skeleton) is from other projects thus not easy to change the indentation because new updates will put it back. I've also preferred to be able to see who made what change over indentation fixes via git blame type commands.

When I can, I try to use the coding style of boost and indentation with four spaces not tabs. Some of the code is from various people/sources, particularly in some non-public repositories that use grl and code from v-rep's distribution.

I’d be happy to switched to a specific coding style, but there would be a couple requirements (which is why I haven’t done this yet):

  1. no manual changes should be needed for third party code
  2. add a ci based linter to verify pull requests, preferably with something like the boost/stl coding style. I’m not a fan of the google coding style because it breaks typedefs with boost, and google bans boost.
  3. prevent pushes directly to master with something like https://gist.github.com/pixelhandler/5718585 or the related script https://gist.github.com/mattscilipoti/8424018
  4. provide instructions so people can check this locally before they push
  5. finally submit the pull request that updates all the relevant code

As you can see this will take some time… so I haven’t taken care of it but a contribution is definitely welcome. Once an approach has been discussed and agreed upon plus a pull request made, I can do steps where project owner permissions are required. :-)

tdinesh commented 7 years ago
  1. prevent pushes directly to master with something like https://gist.github.com/pixelhandler/5718585 or the related script https://gist.github.com/mattscilipoti/8424018

This is from one of the comments in above link

It is worth mentioning that the pre-push hook is a local hook, that is a hook that is fired when invoking a command in the local repository, and is potentially editable by a user. If you want to protect your repositories against disgruntled programmers, a pre-receive hook in the remote repository is necessary. Anyway, thanks for sharing! The script is perfect when you can trust all programmers in the team.

More info on pre-receive hook https://help.github.com/enterprise/2.8/admin/guides/developer-workflow/creating-a-pre-receive-hook-script/

tdinesh commented 7 years ago

When I can, I try to use the coding style of boost and indentation with four spaces not tabs.

Thoughts on 80 column rule? Maybe use two spaces to fit easily in 80 columns?

ahundt commented 7 years ago

When I can, I try to use the coding style of boost and indentation with four spaces not tabs.

Thoughts on 80 column rule? Maybe use two spaces to fit easily in 80 columns?

That's actually one of the parts that bothers me most about google coding style, the contortions needed to fit in 80 characters. It's 2017 so we don't have 640x480 or 720x480 screens anymore... I have a 4k screen. Preferably 320 columns, maybe 160 columns would be okay with me, 80 is definitely too low. Nice visualization of this point below, I don't really need to fit 8 code files side by side. :-)

imagesize

tdinesh commented 7 years ago

Even I think 80 is too low. But need an upperbound and wrap it, 160 seems reasonable.

ahundt commented 7 years ago

On Mon, Feb 13, 2017 at 4:17 PM tdinesh notifications@github.com wrote:

Even I think 80 is too low. But need an upperbound and wrap it, 160 seems reasonable.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ahundt/grl/issues/134#issuecomment-279525091, or mute the thread https://github.com/notifications/unsubscribe-auth/AADZwGVHGXpKRb1nJdxq4txK4iYh8VFwks5rcMhmgaJpZM4L_qe7 .

Sure good enough --

Cheers! Andrew Hundt