code formatting

[cdeil]

remove trailing whitespace in the code and educate developers how to never commit whitespace errors in the future, e.g. http://www.emacswiki.org/emacs/DeletingWhitespace#toc3

[pwaller]

:-P

I'm not offended, though I feel I should point out your language is quite forward. Firstly, I need educating as to why trailing whitespace is an issue. And which trailing whitespace are we talking about? All of it?

    ... stuff here ...
    # Does this count as offending whitespace?
... stuff there ...

[cdeil]

@schmitts Are you planning to make a PR?

If not I can run autopep8 or the Eclipse PyDev auto-formatting on the WebOOT source to do some cleanup.

[schmitts]

If you could run the autopep8 that would be perfect.

@pwaller: Are you fine with it although I did not found the time to
explain why trailing whitespace is a problem?

[pwaller]

Go ahead, though I'd like to look at the result before it is merged.

@schmitts, I'd also like to know more about why you think it is especially a problem. Most sane tools can deal with it without making a huge mess of things. You can do diffs and merging which ignore whitespace. Not to say I think there should be tons of the stuff extraneously. I just don't see why it warrants an issue by itself.

[schmitts]

@pwaller: I use incremental patches a lot (aka add -p). I could not find
a way that ignores whitespace at this step. Do you have one or is your
workflow just different?

[pwaller]

I too use git add -p a lot. Sounds like we may have different workflows, or I'm not understanding something. Does your editor remove this whitespace on save, or something? I don't understand how it could become an issue.

[schmitts]

@pwaller: Sorry, yes, you are right. I do remove the whitespace
automatically which I sometimes forget to disable when working on weboot
code. Apart from that it may happen that I edit some code and later
revert to the original manually. During this I may change the whitespace
by accident. Then, at the 'add -p' step I need to carefully check
whether there is a real change or not.

[pwaller]

ok! Well now I then understand why it's a problem. Hmm. One thing is that I don't use emacs, I tend to use pretty dumb editors like gedit and nano and I don't care enough about the issue to configure them to remove trailing whitespace.

I'd like to add that pep8 doesn't say anything about trailing whitespace, so I don't know if autopep8 will solve the problem.

The more I think about the problem (and I've already wasted 20 minutes thinking about it and reading up on it), I don't really like the idea of adding trailing whitespace to the list of things that has to be cared about. How hard is it to configure emacs not to care for weboot?

There are quite a few problems to solve if you want to keep the option turned on and not have it disturb your workflow. It seems we're not the first to encounter this sort of problem. At the very least I'd need a script with the exact behaviour of emacs otherwise I'd just reintroduce things which emacs thinks are whitespace errors. And then I'd have to take care that the script is working and doing the right thing, and it's another thing which can fail - it would just create more work for me, and others who contribute to the project with minor whitespace errors.

That's just my current feeling, it's not a very strong one, I'm just averse to spending much time on this and I don't see a trivial solution from here which wouldn't involve ongoing busiwork.

[cdeil]

That's a stupid formatting change.

[pwaller]

Wow, the output of autopep8 is not half bad. I've seen much worse in the past (though that was formatting a large C++ code base).

Maybe I'm not so averse after all to enforcing autopep8 compliance via travis. (i.e, builds fail unless they comply).

Re the stupid change, that could equally well be expressed a bit better, or we could change the (forced) line wrapping to, say, 100 rather than 80.

[pwaller]

I'm renaming the issue to "code formatting".

[cdeil]

The pep8 and autopep8 tools do handle trailing and other whitespace:

$ pep8 setup.py
setup.py:38:1: W293 blank line contains whitespace
setup.py:40:17: W291 trailing whitespace
setup.py:52:39: E222 multiple spaces after operator
setup.py:70:17: E203 whitespace before ':'
setup.py:72:17: E251 no spaces around keyword / parameter equals

This is what I get when running autopep8 -i -d weboot.
@pwaller pep8 and autopep8 also have options to ignore certain things.
How do we proceed here?

[pwaller]

Let's do it. This is much less bad than I had in mind.

[cdeil]

There's a few remaining pep8 warnings after running autopep8, because it's not super-agressive with enforcing the line lenght limit:
https://gist.github.com/cdeil/5329972

[pwaller]

@cdeil, would you be willing to fix them?

I'm a big fan of line length limits on the one hand, but I do think there are rare occasions where going over 80 is okay, so we could bump the warning length up to 80?

[cdeil]

Running the Eclipse PyDev code analysis on WebOOT gives these errors and warnings: https://gist.github.com/cdeil/5329975

There's a lot of false positives (e.g. all the undefined variable from import because ROOT.something cannot be found with static code analysis), but also a few real things that should be fixed. Should I make a list of things I think are real and then @pwaller you fix them (I'm afraid if I modify code I haven't read I'll introduce errors).

[pwaller]

s/80/100/

[pwaller]

Okay, if you give me a list of real things I'll take a shot.

[cdeil]

Hold off merging ... I'm manually fixing the 100+ character lines so that pep8 runs cleanly.

[cdeil]

@pwaller Please review.

[pwaller]

@cdeil, test failure due to corrupt tarfile.

Does the code change when you run autopep8? i.e, is it feasible to run it automatically and it won't result in large code changes?

[cdeil]

autopep8 does no further code changes. pep8 complains about this one comment in multitraverse.py. Can it be removed?

$ autopep8 -r weboot/ setup.py -d --max-line-length=100
$ pep8 weboot/ -r --max-line-length=100   
weboot/views/multitraverse.py:201:101: E501 line too long (140 > 100 characters)
weboot/views/multitraverse.py:206:101: E501 line too long (153 > 100 characters)

The travis build error is a glitch because the ROOT tarball download didn't work. I re-triggered the build and it passed.

[pwaller]

okay, you can remove it. Actually you can remove the continue statement and everything below in that for loop.

[cdeil]

Done in https://github.com/cdeil/WebOOT/commit/fb29fc45c9add80fa911e40c63d3be515afadca4 .

[pwaller]

Could you also provide an autopep8 configuration and maybe a make pep8? Is that doable? It'd be nice to make it reproducible without too much effort.

[cdeil]

make pep8 sure. What do you mean with "autopep8 configuration"? You mean a failing travis build?

[pwaller]

I was assuming you'd had to somehow feed some parameters to pep8. If not, then just make pep8 would be great.

[cdeil]

I feed the parameters to pep8 and autopep8 on the command line.
Added a Makefile ( https://github.com/cdeil/WebOOT/commit/f17d98c838f7a370946a92d01117d7657b1619c6 ) wo that we don't have to remember.

[cdeil]

Wait ... let me add that to the travis build so that it fails on non-PEP8. :-)

[cdeil]

@pwaller Please review.

[pwaller]

Travis failed... looks like an old version of pep8?

I'm very happy with this commit. We're good once travis is passing.

[pwaller]

One question, how to apply the diff that autopep8 spits out?

[cdeil]

Use the autopep8 -i instead of -d option to apply the diff in-place. It's a pretty nice tool ... try -h.

[pwaller]

ah, so you'd recommend copying the command that make autopep8 and running it yourself with -i? Maybe we could have a make autopep8-i ?

[cdeil]

The only reason I put -d for diff instead of -i for in-place changes is that I didn't want anyone to accidentally perform changes they didn't want. But either way is fine with me ... changed to -i in the Makefile.

And the travis build passes now that I pip-install an up-to-date version of the pep8 tool.

[pwaller]

Great! 🍻