Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reformat codebase with the Black code formatter #877

Merged
merged 4 commits into from Jul 4, 2020

Conversation

stephenfin
Copy link
Contributor

Now that we've got tests passing consistently on different platforms, it's time to start fixing various bugs and closing feature gaps.
Before we do that though, we need to do something about the code style, which is in a somewhat sorry state.
While one should generally avoid applying large, style-only changes to existing code bases because they tend to make a complete mess of our ability to use 'git-blame' and related tools effectively, without this we'd otherwise be fixing this stuff manually for years.
Bite the bullet and run all the code through black [1] (specifically, black -S, to avoid unnecessary conversion of single quotes to double quotes) to address the worst of the issues.
A future change will address the remaining issues like use of wildcard imports and commented out (dead) code.

Note that black defaults to a max line length of 88, not 79 [2], which means we need to configure flake8 to allow this.

Closes: #724
Closes: #711

@akrabat akrabat changed the title Blackify code Reformat codebase with the Black code formatter Jun 20, 2020
@akrabat
Copy link
Member

akrabat commented Jun 20, 2020

Sorry, @stephenfin - looks like the python3-ification branch conflicted.

One should generally avoid applying large, style-only changes to
existing code bases because they tend to make a complete mess of our
ability to use 'git-blame' and related tools effectively. With that
said, the code base is in pretty bad shape owing to its sheer age and
we'd otherwise be fixing this stuff manually for years. Bite the bullet
and run all the code through black [1] (specifically, 'black -S', to
avoid unnecessary conversion of single quotes to double quotes) to
address the worst of the issues. A future change will address the
remaining issues like use of wildcard imports and commented out (dead)
code.

Note that black defaults to a max line length of 88, not 79 [2], which
means we need to configure flake8 to allow this.

[1] https://pypi.org/project/black/
[2] https://black.readthedocs.io/en/stable/the_black_code_style.html

Signed-off-by: Stephen Finucane <stephen@that.guru>
Closes: rst2pdf#724
Closes: rst2pdf#711
@stephenfin
Copy link
Contributor Author

Rebased

@akrabat
Copy link
Member

akrabat commented Jun 27, 2020

Can we add a note to the DEVELOPERS.rst for the exactly black command that a contributor should use in order to format their code before raising the PR?

Also, can we validate that the code is formatted correctly using Travis?

@akrabat
Copy link
Member

akrabat commented Jun 27, 2020

My first guess on a travis check is black --skip-string-normalization --check rst2pdf

Edit: Should we include --target-version py36?

@stephenfin
Copy link
Contributor Author

Working on it

Mostly end of file/whitespace fixes.

Signed-off-by: Stephen Finucane <stephen@that.guru>
The pre-commit tool [1] can be used to validate black automatically
before committing, saving the need to do so manually. Integrate it,
initially for black and some other basic tests, with an aim to extend to
flake8 and others later.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Nothing too funky here. This is based on what black itself does [1].

[1] https://github.com/psf/black/blob/master/.travis.yml

Signed-off-by: Stephen Finucane <stephen@that.guru>
@stephenfin
Copy link
Contributor Author

This is done now. I integrated pre-commit to automate the process of these locally. Let me know if anything doesn't make sense.

Copy link
Member

@akrabat akrabat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I agree about the pain of a mass-reformatting, but the benefits outweigh the nuisance especially since git blame now supports --ignore-rev / --ignore-revs-file

@akrabat akrabat merged commit 2cdc928 into rst2pdf:master Jul 4, 2020
@akrabat akrabat added this to the 1.0 milestone Jul 4, 2020
@stephenfin stephenfin deleted the blackify branch July 18, 2020 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix flake8 issues across the codebase Formatting of the codebase is bad
2 participants