PEP-8 Reformat and language level bug hunt

[ghtdak]

PyCharm was flagging much unhappiness with tabs and other straightforward formatting issues. I've identified a number of code-level problems including uninitialized variables, missing include directives (e.g. time), unused includes, variable shadowing etc but am early in the process of identifying them all.

The first commit (f2dbf15) included significant manual reformatting to lib/common.py, lib/irc.py and tumbler.py as well as file-level automated reformatting.

The second commit (097e07fc72) is a bulk / automated reformat. I pushed this because the massive whitespace / import organization changes will be a distraction going forward.

I'm willing to go through and manually adjust for PEP-8 compliance but merging the bulk reformatting would make life easier going forward.

I recommend adding an http://editorconfig.org file which I can add later (once I understand it better :-)

[chris-belcher]

I agree in principle but reformatting like this will break lots of git tools like blame.

I regret using tabs instead of spaces for what its worth.

I'm interested in the views of all the other contributors for how we could go forward with this issue.

[adlai]

Showing 39 changed files with 7,065 additions and 6,177 deletions.

I'm all for standardized formatting, but this will take a while to review manually.

For starters, @ghtdak, could you provide the command you used for the automated formatting, so we can run it independently for comparison?

Edit: @chris-belcher's point about git blame is an excellent one, and I'm swinging towards a NACK.

[ghtdak]

git blame has a flag (-w) to skip changes due to whitespace. I used tig to view the changes which also allows varying whitespace change display. Without whitespace, the number of lines to view 13514 to 4706. Scrolling down through the changes is (reasonably) straightforward and its easy to verify that nothing of substance has changed (famous last words :-)

While its certainly possible that errors have been introduced, I've yet to see PyCharm introduce an error from reformatting.

In terms of the command I used: In PyCharm you can select a directory hierarchy and apply reformatting / include fixes without opening each file.

I won't rehash why PEP-8 is good but this is the first Python project I've seen which uses formatting like this and I'm guessing this will be a issue going forward.

[chris-belcher]

I didn't know about -w, that's good. I haven't been able to make it work on the github web interface though.

According to this it should be possible to run blame from a given commit, which is another way for blame to continue working. Although it seems the current github doesn't have that feature (or maybe i need to pay for enterprise to get it)

We could use autopep8 to independently get pep8 formatted code?

[dcousens]

git blame has a flag (-w) to skip changes due to whitespace.

Well that is awesome.
In any case, the excuse that formatting destroys git blame is bollocks. You can always just git blame with a HEAD set at before it was formatted.
Its only a persistent issue if formats are repeatedly done.

[ghtdak]

On Sat, Nov 14, 2015 at 11:32 PM, marcoagner [email protected]
wrote:

I'd like to know if there was any final decision agreed on this issue. I
consider this an important topic and think this may help future
contributions.
Thanks!

From a best practices / software engineering perspective the code is
awful. In the community, however, there's no consensus this is a problem
which needs fixing.

My efforts are in the commit history of:

https://github.com/ghtdak/joinmarket/commits/master-refactor_v1

which are applied after the blame-preserving Yapf reformat:

https://github.com/ghtdak/gitreformat