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

WIP [REFACTORING] Use black for autoformatting #4453

Closed
wants to merge 8 commits into from

Conversation

jtwaleson
Copy link
Collaborator

Description

To prevent needless discussions on code style, pep8/pyflakes (together flake8) is already used to enforce a coding standard and prevent python errors. The program black makes things even simpler and completely standardizes the way that python code is formatted. This PR does a couple of things:

  • Add black to pre-commit hook
  • Add black to github actions to check all code and give an error if it does not agree
  • Applies black to all existing code
  • Changes a flake8 rule (E203) that is not 100% compatible with black. See Does not comply with E203 from pep8 psf/black#315 .

Applying black to all code is a MASSIVE change, so don't hesitate to pause this PR and see what the rest of the community thinks.

Pros for applying black:

  • no more discussion on how to format things in python code
  • super quick cleanup by just running the pre-commit hook

Cons:

  • for newcomers it's an additional thing they might need to get familiar with
  • it messes a bit with the git history. This PR changes most of the python files with purely cosmetic changes.

It's a bit of a mix between a CHORE and REFACTORING. No code is functionally changed. For the code review, only look at commit 836a027 as editing the test code to not use lambdas was done by hand.

Mentioned in discussion #4447

What more can I say.

How to test

To verify nothing else was changed do the following:

  • install black on your system
  • check out the repo at the commit before "[CHORE] massive change - apply black for autoformatting"
  • run black
  • run git diff c4ea7b6b14cd2c18cf00a87fb884bbb623849286 (currently that is the commit in this PR that applies black)
  • there should be no diff, indicating that

Checklist

  • Contains one of the PR categories in the name
  • Describes changes in the format above
  • Links to an existing issue or discussion
  • Has a "How to test" section

@jtwaleson jtwaleson changed the title [REFACTORING] Use black for autoformatting WIP [REFACTORING] Use black for autoformatting Sep 3, 2023
@jtwaleson
Copy link
Collaborator Author

Still needs some work as this clashes with autopep8.

@Felienne
Copy link
Member

Felienne commented Sep 3, 2023

Still needs some work as this clashes with autopep8.

Yes, that too does some auto-rewriting. I am not entirely sure what black adds over autopep (there probably are if you suggest this) but it would be nice to add to this PR so we can discuss the value.

jtwaleson and others added 2 commits September 4, 2023 12:17
We should not use autopep8 and black at the same time, as they have the
same goal. This removes autopep8 from the checkers.
@Felienne
Copy link
Member

Felienne commented Sep 7, 2023

Let's close this for now, it does not add enough value to warrant changes in working and conflicts in other open PR's.

@Felienne Felienne closed this Sep 7, 2023
@jtwaleson jtwaleson deleted the use-black branch September 10, 2023 11:05
@jtwaleson jtwaleson restored the use-black branch September 10, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants