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

Code cleanup #134

Open
ahundt opened this issue Feb 13, 2017 · 5 comments
Open

Code cleanup #134

ahundt opened this issue Feb 13, 2017 · 5 comments
Projects

Comments

@ahundt
Copy link
Owner

ahundt commented Feb 13, 2017

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
Copy link
Contributor

tdinesh commented Feb 13, 2017

  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
Copy link
Contributor

tdinesh commented Feb 13, 2017

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
Copy link
Owner Author

ahundt commented Feb 13, 2017

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
Copy link
Contributor

tdinesh commented Feb 13, 2017

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

@ahundt
Copy link
Owner Author

ahundt commented Feb 13, 2017 via email

@ahundt ahundt added this to TODO Long Term in Plan Sep 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Plan
TODO Long Term
Development

No branches or pull requests

2 participants