I propose addition to QA bot - check for trailing whitespace in patches.
Probably checking for tabs in Java files would be also good idea.

On Sun, Nov 25, 2012 at 11:14 AM, Radim Kolar <[EMAIL PROTECTED]> wrote:

> I propose addition to QA bot - check for trailing whitespace in patches.
> Probably checking for tabs in Java files would be also good idea.
>
>
Checking for tabs I could get behind; checking for trailing whitespace not
so much.

I've never understood why folks get worked up over a little trailing
whitespace here and there, since you can't see it and it doesn't affect
correctness. Spurious whitespace changes that make a review harder - those
are annoying. Trailing whitespace inadvertently left on lines where
legitimate changes were made in a patch - doesn't seem too harmful to me.

--
Aaron T. Myers
Software Engineer, Cloudera

> I've never understood why folks get worked up over a little trailing
> whitespace here and there, since you can't see it and it doesn't affect
> correctness. Spurious whitespace changes that make a review harder - those
> are annoying. Trailing whitespace inadvertently left on lines where
> legitimate changes were made in a patch - doesn't seem too harmful to me.

Trailing whitespace is annoying because:
     if you have editor to set killing it, it will produce large patch.
     if use use scroll up at end of line, then cursor will not jump to
end of text but some space after it, it cost you more clicks for cursor
movement and it is annoying if it ends of split line.
     its good and standard practise to avoid it, git and other tools
highlight it in red.
     if you use ignore whitespace in git diff, it often produces patch
failing to apply

Trailing whitespace can be striped by pre-commit hook.

OK, if folks want to do something to get rid of trailing whitespace in the
project I won't object, but it doesn't seem like that big a deal to me. A
pre-commit hook makes sense to me. I just don't want to see the QA bot flag
patches containing trailing whitespace, thus requiring more round trips on
patches.

--
Aaron T. Myers
Software Engineer, Cloudera

On Mon, Nov 26, 2012 at 10:53 AM, Radim Kolar <[EMAIL PROTECTED]> wrote:

>  I've never understood why folks get worked up over a little trailing
>> whitespace here and there, since you can't see it and it doesn't affect
>> correctness. Spurious whitespace changes that make a review harder - those
>> are annoying. Trailing whitespace inadvertently left on lines where
>> legitimate changes were made in a patch - doesn't seem too harmful to me.
>>
>
> Trailing whitespace is annoying because:
>     if you have editor to set killing it, it will produce large patch.
>     if use use scroll up at end of line, then cursor will not jump to end
> of text but some space after it, it cost you more clicks for cursor
> movement and it is annoying if it ends of split line.
>     its good and standard practise to avoid it, git and other tools
> highlight it in red.
>     if you use ignore whitespace in git diff, it often produces patch
> failing to apply
>
> Trailing whitespace can be striped by pre-commit hook.
>

Some projects have a subversion pre or post-commit hook to check for
and/or correct these kinds of issues.

Inconsistent line endings (CRLF/LF), bogus tabs, and extra whitespace
are all things we could check for and/or correct this way.

Colin
On Mon, Nov 26, 2012 at 9:44 AM, Aaron T. Myers <[EMAIL PROTECTED]> wrote:
> On Sun, Nov 25, 2012 at 11:14 AM, Radim Kolar <[EMAIL PROTECTED]> wrote:
>
>> I propose addition to QA bot - check for trailing whitespace in patches.
>> Probably checking for tabs in Java files would be also good idea.
>>
>>
> Checking for tabs I could get behind; checking for trailing whitespace not
> so much.
>
> I've never understood why folks get worked up over a little trailing
> whitespace here and there, since you can't see it and it doesn't affect
> correctness. Spurious whitespace changes that make a review harder - those
> are annoying. Trailing whitespace inadvertently left on lines where
> legitimate changes were made in a patch - doesn't seem too harmful to me.
>
> --
> Aaron T. Myers
> Software Engineer, Cloudera

IIRC we still have a checkstyle section in the QA bot's checking
script, if that still is interesting for devs to turn on again. At
some point it was probably disabled.

On Mon, Nov 26, 2012 at 12:44 AM, Radim Kolar <[EMAIL PROTECTED]> wrote:
> I propose addition to QA bot - check for trailing whitespace in patches.
> Probably checking for tabs in Java files would be also good idea.

--
Harsh J