Locust: Let's fix code to be PEP8 compliant?

[max-rocket-internet]

Is your feature request related to a problem? Please describe.

I am working on a couple PRs for locust and my IDE automatically removes trailing spaces and whitespace from blank lines. This means when I want to make a change to a file in the repo it also edits many lines to "fix" them. It's a bit annoying as I then have to go and undo all the changes. I know I could disable the feature but it has good intentions 😃

Example output from pycodestyle:

$ pycodestyle locust/exception.py
locust/exception.py:4:1: E302 expected 2 blank lines, found 1
locust/exception.py:7:1: E302 expected 2 blank lines, found 1
locust/exception.py:10:1: E302 expected 2 blank lines, found 1
locust/exception.py:13:1: E302 expected 2 blank lines, found 1
locust/exception.py:17:1: W293 blank line contains whitespace
locust/exception.py:20:80: E501 line too long (91 > 79 characters)
locust/exception.py:25:1: E302 expected 2 blank lines, found 1
locust/exception.py:28:1: E302 expected 2 blank lines, found 1
locust/exception.py:31:1: W293 blank line contains whitespace
locust/exception.py:32:80: E501 line too long (86 > 79 characters)
locust/exception.py:32:87: W291 trailing whitespace
locust/exception.py:33:80: E501 line too long (85 > 79 characters)
locust/exception.py:36:1: E302 expected 2 blank lines, found 1
locust/exception.py:38:80: E501 line too long (81 > 79 characters)
locust/exception.py:41:1: E302 expected 2 blank lines, found 1
locust/exception.py:48:1: E302 expected 2 blank lines, found 1
locust/exception.py:55:1: E302 expected 2 blank lines, found 1

Describe the solution you'd like

Fix all current formatting so it's PEP8 compliant and then add a Github action to enforce it.

[cyberw]

I want to go even further and use Black for autoformatting (and adding it to the build as well). I think I even added it as a ticket a long time ago, not sure where it is though.

[heyman]

Do you know if your editor support .editorconfig file? In that case this could be a solution: https://github.com/locustio/locust/pull/759#issuecomment-547862222

[max-rocket-internet]

But the problem is the locust code, not my editor. My editor is cleaning it up, and I know I can disable this feature, but it should be clean already 🙂

[heyman]

But what's the real drawback from having trailing whitespaces? When is it actually a problem (apart from when your editor is cleaning it up)?

A real drawback from removing whitespace from blank lines is when you try to copy and paste a function into an interactive shell. Definitely not a huge one, but it happens and it's something I run into from time to time.

The point with a .editorconfig file - and the reason why I mentioned it - is that you can disable it just for a specific project, and don't have to disable it for the whole editor.

[max-rocket-internet]

My beef is not particular with trailing whitespace, it's just one example of a cosmetically messy code, of which there is a lot in here. There is already well defined formatting guidelines and tools to fix/enforce it because it makes working on the code base a more enjoyable experience for everyone.

If you really want trailing whitespace, then no worries, just make it consistent, add it to every line and add configuration to Black (or whatever tool) to check it and keep it consistent 🙂

[heyman]

If I could go back in time and turn on some very basic formatting checks back when this project started, I would :). I just don't think it's a huge problem that there is some formatting inconsistency, and I don't think it's worth obfuscating the 10 year git history in order to achieve formatting consistency.

[rafaelrds]

It is possible to migrate the code style with Black without ruining git blame by ignoring revisions with --ignore-revs-file flag. [[source](https://github.com/psf/black#migrating-your-code-style-without-ruining-git-blame)]

[HeyHugo]

It's possible to run:
git filter-branch --tree-filter 'black . || echo "Invalid python probably :("' -- --all

This will replay the full git history and apply black formatting everywhere (except when there are python syntax errors). The above should take something like an hour to run. It can probably be made faster by filtering out just changed/added files though.

The big drawback here is history is rewritten which would force any collaborators to re-clone or hard reset their local repos. Also rewriting history like this is pretty scary 😬
..so maybe a dealbreaker?

Cred: https://medium.com/millennial-falcon-technology/reformatting-your-code-base-using-prettier-or-eslint-without-destroying-git-history-35052f3d853e

[cyberw]

It's possible to run:
git filter-branch --tree-filter 'black . || echo "Invalid python probably :("' -- --all

This will replay the full git history and apply black formatting everywhere (except when there are python syntax errors). The above should take something like an hour to run. It can probably be made faster by filtering out just changed/added files though.

The big drawback here is history is rewritten which would force any collaborators to re-clone or hard reset their local repos. Also rewriting history like this is pretty scary 😬
..so maybe a dealbreaker?

Cred: https://medium.com/millennial-falcon-technology/reformatting-your-code-base-using-prettier-or-eslint-without-destroying-git-history-35052f3d853e

Wow. That's really cool/crazy :) But I'm sure there will be some commits with broken code that Black cant format which will make it blow up, so it is probably not an option even if we were crazy :)

[HeyHugo]

@cyberw I did a test run and there's very few commits that has broken python files. When that happen those files are simply not formatted during that commit but when a later commit makes them valid python again they are formatted.

So the "_only_" thing that might be too painful is the history rewrite for collaborators, existing PR's etc 😅

The run took 1 hour and 22 minutes
Pushed the formatted master here if you want to see: https://github.com/HeyHugo/locust

[cyberw]

@cyberw I did a test run and there's very few commits that has broken python files. When that happen those files are simply not formatted during that commit but when a later commit makes them valid python again they are formatted.

So the "_only_" thing that might be too painful is the history rewrite for collaborators, existing PR's etc 😅

The run took 1 hour and 22 minutes
Pushed the formatted master here if you want to see: https://github.com/HeyHugo/locust

Cool stuff! But the blame/line history for any file that was ever broken would be pretty messed up, and that might actually make it worse than @rafaelrds suggestion.

Not to mention the poor sods that are trying to maintain a fork (not sure how many of those there are)

[HeyHugo]

Cool stuff! But the blame/line history for any file that was ever broken would be pretty messed up, and that might actually make it worse than @rafaelrds suggestion.

Yeah I'm not sure how that would look 🤔

Not to mention the poor sods that are trying to maintain a fork (not sure how many of those there are)

Heh yes good point.

[heyman]

Interesting! The drawbacks from the history rewrite solution probably outweighs the benefits. I imagine any mentioned commit hash in github comments, line comments in PRs, etc. will probably get lost as well.

I didn't know about --ignore-revs-file, and it should definitely be helpful - especially if Github implements support for it at some point.