Jonathan Eagles
added a comment - 26/May/11 21:08 Todd, I wondering if we should remove the '-c' on the first grep. I'm in favor of coding style checking. Do you think we have a need to do something like checkstyle in the future?

You're absolutely right about the first -c.. I was testing with a patch that had only one tab so I didn't notice

Something like checkstyle probably makes sense at some point, but right now we have so many violations that it would be a big change. Spaces vs tabs is one thing everyone agrees on and where there's never any question of "aesthetics" So let's start here and maybe some day ease in some checkstyle.

Todd Lipcon
added a comment - 27/May/11 03:01 You're absolutely right about the first -c.. I was testing with a patch that had only one tab so I didn't notice
Something like checkstyle probably makes sense at some point, but right now we have so many violations that it would be a big change. Spaces vs tabs is one thing everyone agrees on and where there's never any question of "aesthetics" So let's start here and maybe some day ease in some checkstyle.

Tsz Wo Nicholas Sze
added a comment - 27/May/11 03:06 Nice work! We should have this long time ago.
BTW, do you want to also check 80 characters per line? But it is arguably if it is the rule we want to enforce.

IMO 80 chars per line is one of those things that should be kept in general, but if the occasional line flows over it's not a big deal. There's some times when it's much less readable to try to keep to it.

But this is where we get into religious wars I'm a 100-per-line devotee myself!

Todd Lipcon
added a comment - 27/May/11 03:14 IMO 80 chars per line is one of those things that should be kept in general, but if the occasional line flows over it's not a big deal. There's some times when it's much less readable to try to keep to it.
But this is where we get into religious wars I'm a 100-per-line devotee myself!

> Do you think we have a need to do something like checkstyle in the future?

We have long had a checkstyle target in ant, and plots of checkstyle violations over time (see https://builds.apache.org/job/Hadoop-Common-trunk/), but unfortunately we haven't enforced the rule that the number may not increase (unlike javadoc or findbugs warnings for example).

Another way of implementing this JIRA would be to enable such a rule (perhaps with a weaker set of checkstyle rules than the current set, e.g. drop the 80-per-line rule).

Tom White
added a comment - 27/May/11 05:26 > Do you think we have a need to do something like checkstyle in the future?
We have long had a checkstyle target in ant, and plots of checkstyle violations over time (see https://builds.apache.org/job/Hadoop-Common-trunk/ ), but unfortunately we haven't enforced the rule that the number may not increase (unlike javadoc or findbugs warnings for example).
Another way of implementing this JIRA would be to enable such a rule (perhaps with a weaker set of checkstyle rules than the current set, e.g. drop the 80-per-line rule).