686547 - Trim trailing white space from the source code files

Status: RESOLVED FIXED

(Socorro :: General, task)

Description

[Lonnen :lonnen]

Both our python and php code stye guides discourage trailing whitespace. As part of our larger code cleanup effort we could script removing trailing spaces from uncompressed files in version control. This would knock out more than half of our pep 8 violations alone. Following this change all new code should be trailing white space free. We could script enforcement of this, even.

Comment 1

[Lonnen :lonnen]

Https://github.com/mozilla/socorro/pull/26



R?

Comment 2

[Robert Helmer [:rhelmer]]

Hrm I see jar and other files in this, which you probably don't want to modify in this way :)

Could you rerun this only against text files? Probably just *.php and *.py would be enough - if you want to do all text files (I assume you are scripting this), the unix "file" command has pretty good heuristics for checking for text files (looks for magic headers, unprintable chars etc)

Comment 3

[Lonnen :lonnen]

Removed the modifications to the binary stuff. 

https://github.com/mozilla/socorro/pull/29

Comment 4

[Robert Helmer [:rhelmer]]

(In reply to Chris Lonnen :lonnen from comment #3)
> Removed the modifications to the binary stuff. 
> 
> https://github.com/mozilla/socorro/pull/29

I think this is fine, the only thing I can think of is - would this hurt any multi-line strings like:
https://github.com/Lonnen/socorro/commit/691aa9978807be1ff98f80e233c877d8bfc9926a#L237R73

I don't *think* it should because SQL understands line breaks, should be fine. I'll merge and we should test that.

The only other thing is that doing this on schema dumps (or any other generated file) is probably futile since it'll keep coming back.

Also, now that you've done this work, can we put a check for Jenkins to run to make sure it doesn't regress? Would you mind filing a bug for that (assuming you agree)?

Running a pep8 checker should help, but we might consider just running a generic one across all text files (perhaps with a blacklist of files like the pg schema).

Comment 5

[Lonnen :lonnen]

(In reply to Robert Helmer [:rhelmer] from comment #4)
> I think this is fine, the only thing I can think of is - would this hurt any
> multi-line strings like:
> https://github.com/Lonnen/socorro/commit/
> 691aa9978807be1ff98f80e233c877d8bfc9926a#L237R73

I looked for multiline strings when I proofed the changes and I didn't catch any that would be problematic. The sql strings should be fine. I was able to run things from my branch without a fuss, but I hardly went through all the pages.

> The only other thing is that doing this on schema dumps (or any other
> generated file) is probably futile since it'll keep coming back.

White list it in our checker (linter?) or implement pre-commit hooks that trim for us. The latter is more difficult and adds complexity.
 
> Also, now that you've done this work, can we put a check for Jenkins to run
> to make sure it doesn't regress? Would you mind filing a bug for that
> (assuming you agree)?

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=686829

> Running a pep8 checker should help, but we might consider just running a
> generic one across all text files (perhaps with a blacklist of files like
> the pg schema).

Other projects use https://github.com/jbalogh/check, which will run pyflakes, PEP8, and white space checking. We're pretty far from PEP 8 compliance, though. We also might need to fork it to add white listing. Not sure if it would be better to use this now and put up with the warnings or to write a few lines of bash to check for white space and add the python stuff when it's ready... any more discussion in that line of thinking should probably go in the aforelinked bug, though.