Keep Commits Clean (a pet peeve)

by debianjoe

greenlamps

Having just spent the last few hours reordering and recommenting a little group project, I felt the need to bring up a few things that drive me totally nuts about the FOSS world from time to time.  I am not particularly obsessive compulsive, but there are a few things that I think that everyone who works on a project should take into consideration.  If I’m getting help or working as a team on a project, I don’t care if someone has taken a bath today, as I’m probably working thousands of miles away from them.  I don’t care if their house is a mess.  I DO care, though, if they’re coding is clean.

There are standards in place for many languages, and there are agreed upon “best practices”, but sometimes, there are projects that fall outside of those areas.  If there is not an agreed upon standard, then it’s up to the team working on it to make one.  All that should matter is that there’s a standard, and not just “code that the compiler/interpreter will use.”  In many cases, if someone’s work is worthwhile, I’ll accept a pull request and try to clean up some variations on my own.  Trailing whitespace is acceptable if the work is significant, but I have to know that someone was just too occupied with trying to solve a problem to pay attention to these little details.  If it’s a pointless commit, and it has unorganized commenting, useless commenting, poor standardization, or unreadable commenting…I won’t even bother with it.

For example:

void my_funtion (int x, int y)
{
    int y, x;
 int f;
/*for loop*/
for (x= 0; x <10; x++)
 do this to int f;
      also do this;

This is an obviously exaggerated example, but I hope that you can see why this would drive someone totally insane if it was a commit to an otherwise well formatted program.

So what are the main things to be concerned with before pushing a commit?

1). That your patch works and hopefully doesn’t cause issues for other users.  Never break something that works.  Test, test, test.

2). That your patch follows the project’s format, and that it maintains consistency to itself as well.  Even if you hate K&R, if that’s what the project that you’re patching on is using, then I’d suggest maintaining the pattern.  On the other hand, you might wish to offer to clean it up for the rest of the team.

3). That you’ve deleted trailing white-space from your patch.  This is a huge pet-peeve, but it drives me totally nuts when I switch to white-space mode in emacs.  It’s simply wasted space.  As a fun fact, many Linux kernel developers get started by removing white-space.

4). That your comments make the patch easier to understand.  You should always strive to write comments that add value.  I’ve seen people explain how something in the language works in a comment.  There’s no reason  to do this for a patch, as you can assume that the author knows how the language works.  On the other hand, they may not understand what you’re hoping to accomplish.

5). That your patch is useful and elegant.  Always be attempting a samurai like approach to coding.  If you can make that perfect cut in a minimal piece of code, and it makes exactly the right difference…then people will appreciate and remember your work.

The projects that we work on may die out, or they may be forked and picked up by others.  Always remember that there is the possibility that your work won’t end with you.  Try to make life easy for those that come behind you.

Advertisements