Sage whitespace policy

[Andrew Mathas]

I have some patches on trac and the patchbot is failing them with white space errors with comments like

Trailing whitespace inserted on 73 non-empty lines.

I used to have my editor set to automatically kill off all trailing white space but I found that this led to constant conflicts when merging so now I leave white space alone. As the patchbot complains I am guessing this is not sage's policy either.

What is the accepted best practice when writing patches with respect to white space? This does not seem to be mentioned in the developers guide and the only comments that I have found about this was an unresolved discussion about whether all trailing white space was bad, or whether trailing white space up to the previous indent level was OK. Is there a list of recommended editor settings (I live on the vi-side).

Cheers,
Andrew

[Jeroen Demeyer]

I don't think there is an official policy, but at the very least you
should not introduce new trailing whitespace.

Certainly don't go to the other extreme: don't simply remove all
trailing whitespace everywhere, because that does indeed lead to patch
conflicts.

My recommendation is: remove trailing whitespace in parts of the code
that you're editing, i.e. where the changes would lead to patch
conflicts anyway.

[Keshav Kini]

Andrew Mathas <andrew...@gmail.com> writes:
> I have some patches on trac and the patchbot is failing them with
> white space errors with comments like
>
>
> Trailing whitespace inserted on 73 non-empty lines.
>
>
> I used to have my editor set to automatically kill off all trailing
> white space but I found that this led to constant conflicts when
> merging so now I leave white space alone. As the patchbot complains I
> am guessing this is not sage's policy either.

The patchbot is complaining about trailing whitespace that *you* added.
I think that's pretty compatible with a policy of "leaving whitespace
alone", since actively adding white space isn't exactly leaving it alone
:)

-Keshav

----
Join us in #sagemath on irc.freenode.net !

[Jeroen Demeyer]

On 2012-08-13 11:20, Keshav Kini wrote:
> The patchbot is complaining about trailing whitespace that *you* added.

That's not completely true. I think it also complains about whitespace
on lines which were edited but where the trailing whitespace wasn't removed.

[Harald Schilly]

On Monday, August 13, 2012 10:33:27 AM UTC+2, Andrew Mathas wrote:

Trailing whitespace ...

Hi, I don't know what it is actually doing, but from my experience, it helped a lot to tweak my vim a bit. Now, it prints a small centered dot for each space (that is not "ok"), and an arrow to the right for each TAB.

" tab chars and tailing spaces
set list listchars=tab:▷⋅,trail:⋅,nbsp:⋅

Those arrow and dot signs are UTF8, I hope this comes through.

H 

[Volker Braun]

We don't have an official policy, so this is what I think. The Sage library has trailing whitespace all over the place, and trying to enforce any kind of whitespace policy without automation through commit hooks just combines the disadvantages. 

  * The whitespace plugin in the patchbot is purely advisory (and not an error)

  * You should not take trailing whitespace into account when reviewing patches.

  * Do not make whitespace visible in your editor (unless its your personal preference)

  * Do not make whitespace-only changes when you create patches

 

Basically, its hard enough to get anything done without bickering over whitespace. So just leave it. Maybe we'll globally chomp off trailing whitespace when we move to git, but until then don't worry about it.

[P Purkayastha]

On 08/13/2012 06:09 PM, Harald Schilly wrote:
>
>
> On Monday, August 13, 2012 10:33:27 AM UTC+2, Andrew Mathas wrote:
>
> Trailing whitespace ...
>

The patchbot probably checks for trailing whitespace on all the lines
starting with '+' in the patch. So, it will catch all the lines which
have been edited and still retain a trailing whitespace. This is
irrespective of whether the spaces were introduced in the current patch
or were present previously.

>
> Hi, I don't know what it is actually doing, but from my experience, it
> helped a lot to tweak my vim a bit. Now, it prints a small centered dot
> for each space (that is not "ok"), and an arrow to the right for each TAB.
>
> ||
> " tab chars and tailing spaces
> set list listchars=tab:▷⋅,trail:⋅,nbsp:⋅
>
>
> Those arrow and dot signs are UTF8, I hope this comes through.
>
> H

I am using the ShowTrailingWhiteSpaces script:
http://www.vim.org/scripts/script.php?script_id=3966

It is painless and requires no configuration. The indicators disappear
on insert mode, in the currently editing line, so it is not annoying
either. Example screenshot: http://i.imgur.com/fyyPh.png

[Robert Bradshaw]

Yep, exactly. This of course includes lines you edited, but didn't add
whitespace to (as these would be conflicts anyways).

Volkers point is right on: the patchbot is purely advisory. The more
information when reviewing a patch the better, but it is not the
arbitrator, the human reviewer is. My personal thought is that we
should avoid trailing whitespace, but not make sweeping changes until
we have the tools to cleanly deal with it.

- Robert

[Marco Streng]

2012/8/14 Robert Bradshaw <robe...@math.washington.edu>:

> Volkers point is right on: the patchbot is purely advisory.

Then I suggest a small change to the patchbot.

iirc, the patchbot (and its blob on trac) shows a huge "plugin failed"
when there are trailing whitespaces on changed/added lines, without
saying anything about whether the Sage doctests pass. You have to open
the log file to see the result of the doctests. Can we make the
doctest results more prominent again (like they were before the
introduction of this plugin)?

[Robert Bradshaw]

TestsPassed < PluginFailed, so it will only ever be blue if all tests
passed but one or more plugins failed, so need to open the log for
this.

- Robert

[Keshav Kini]

so *no* need to open the log for this, I assume you mean.

[Marco Streng]

2012/8/14 Robert Bradshaw <robe...@math.washington.edu>:

> TestsPassed < PluginFailed, so it will only ever be blue if all tests

I assume this "<" is an implication arrow to the left. So ok, forget
about my suggestion and thanks for the explanation!

[Kwankyu Lee]

Hi,

I use the following script to check newly added trailing whitespaces just before I upload a patch. It uses the same code in the patchbot trailing whitespaces plugin. :-) Just put it into an executable file, say, "check_spaces"


#!/usr/bin/env python
import sys
import re
import os

def trailing_whitespace(patches, ignore_empty = True):
    bad_lines = 0
    trailing = re.compile("\\+.*\\s+$")
    for patch_path in patches:
        patch = os.path.basename(patch_path)
        print patch
        for ix, line in enumerate(open(patch_path)):
            line = line.strip("\n")
            m = trailing.match(line)
            if m:
                print "    %s:%s %s$" % (patch, ix+1, line)
                if line.strip() == '+' and ignore_empty:
                    pass
                else:
                    bad_lines += 1
    msg = "Trailing whitespace inserted on %s %slines." % (bad_lines, "non-empty " if ignore_empty else "")
    if bad_lines > 0:
        print msg
    else:
        print "All clear."

if __name__ == "__main__":
    if len(sys.argv) > 1:
        patch = sys.argv[1]
        trailing_whitespace(patches=sys.argv[1:])
    else:
        pass