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

Black reformat #7967

Closed
wants to merge 2 commits into from
Closed

Conversation

zerolab
Copy link
Contributor

@zerolab zerolab commented Feb 11, 2022

Fixes #6056

This PR

  • adds black to pre-commit configuration, testing_extras in setup.py, Makefile and lint job in CircleCI configuration.
  • updates flake8 and isort configuration to use the black profile. Note that I left isort at the current version (5.6.4) as profiles support was added in 5.0.0
  • updates editorconfig with the new line length (88) for py files
  • updates python guidelines in docs

and finally, reformats with black (and fixes E501 - line length for updated flake8 config)

Rebasing PRs

Based on the methodology from #7908

# Make a copy of your branch
git branch save/my-existing-branch my-existing-branch
# Make sure you have the latest `main` from Wagtail
git checkout main
git pull upstream main
# Rebase your work onto the version of `main` right before
# we reformatted the whole project with Prettier –
# you can then resolve conflicts that would have been present anyway.
git rebase 6dae6e5d07059a12ec70deec6510b041582a88e7
# Install dependencies
## pre-commit via pip and initialize
pip install pre-commit && pre-commit install
## black/flake8 directly via Wagtail's testing dependencies (either in a VM or a virtual environment)
pip install -e .[testing]
# Run black and flake8
## with pre-commit
pre-commit run black && pre-commit run flake8
## without
black . && flake8 .
# Rebase again, this time with the reformatting as the base,
# always preserving your changes in case of conflicts, and automatically linting and reformatting.
## pre-commit
git rebase \
  --strategy-option=theirs \
  --exec '(pre-commit run black --all-files || true) && git add . && git commit --amend --no-edit --no-verify' \
  d10f15e55806c6944827d801cd9c2d53f5da4186
## or, directly
git rebase \
  --strategy-option=theirs \
  --exec '(black --target-version py37 . || true) && git add . && git commit --amend --no-edit --no-verify' \
  d10f15e55806c6944827d801cd9c2d53f5da4186
# Lint again, this should not show any errors
## using precommit
pre-commit run --all-files
## or, directly
black --target-version py37 .
flake8 .
# Finally rebase onto the latest version from Wagtail main, as per usual.
git rebase main

To-Do

@squash-labs
Copy link

squash-labs bot commented Feb 11, 2022

Manage this branch in Squash

Test this branch here: https://zerolabchoreblack-configuratio-j43q0.squash.io

@zerolab zerolab marked this pull request as draft February 12, 2022 00:50
@zerolab zerolab changed the title Chore/black configuration Black reformat Feb 13, 2022
@zerolab zerolab marked this pull request as ready for review February 13, 2022 16:02
@atombrella
Copy link
Contributor

Please remember to add the commit hash to https://github.com/wagtail/wagtail/blob/main/.git-blame-ignore-revs once merged

@zerolab
Copy link
Contributor Author

zerolab commented Feb 13, 2022

Absolutely. As well as instructions for reading WIP PRs

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

I have a couple questions about the configuration and want to suggest we don’t enforce line lengths with flake8 in addition to black, since it doesn’t seem to be very useful.

In addition, should we use black’s --target-version py37? Trying it out, it was additionally adding trailing commas in a few files.

Finally, with Wagtail having lots of Python code in its documentation, I was wondering if we should run black there (see blackdoc) for consistency? Either in this PR or as a separate follow-up.

docs/contributing/python_guidelines.rst Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
This:
- updates the pre-commit configuration and setup.py testing dependencies
- updates isort/flake8 configuration for black
- adds black linting to Makefile and CircleCI configuration
- updates editorconfig with the new line length (88) for py files
- updates python guidelines in docs
@thibaudcolas
Copy link
Member

Looking good! I get no extra reformatting locally, and it seems like all linting is passing in CI. Note I only reviewed some of the code changes a few commits ago – haven’t re-reviewed the reformatting changes since.

@thibaudcolas thibaudcolas added this to the 2.17 milestone Feb 15, 2022
zerolab added a commit that referenced this pull request Feb 15, 2022
@zerolab
Copy link
Contributor Author

zerolab commented Feb 15, 2022

Merged in d10f15e + parents.
Changelog - 716bf92
.git-blame-ignore-revs - f16d29e

@zerolab zerolab closed this Feb 15, 2022
@zerolab zerolab deleted the chore/black-configuration branch February 15, 2022 13:11
@lb-
Copy link
Member

lb- commented Feb 15, 2022

Great stuff @zerolab

@gasman
Copy link
Collaborator

gasman commented Feb 17, 2022

@zerolab Please can we put the 'rebasing PRs' instructions somewhere more easily findable (a wiki page perhaps), and ensure they're fixed and updated as necessary? (one-commit-before-reformatting doesn't appear to be defined, and I'm not sure it makes sense to rebase on top of it while main is checked out...)

@gasman
Copy link
Collaborator

gasman commented Feb 17, 2022

@zerolab Please can we put the 'rebasing PRs' instructions somewhere more easily findable (a wiki page perhaps), and ensure they're fixed and updated as necessary? (one-commit-before-reformatting doesn't appear to be defined, and I'm not sure it makes sense to rebase on top of it while main is checked out...)

Never mind - piecing it together from the instructions on #6059 (comment) now. Will create that wiki page when I'm done :-)

@zerolab
Copy link
Contributor Author

zerolab commented Feb 17, 2022

Gah, I missed one-commit-before-reformatting. Added in the code block in the first comment (the sha was 6dae6e5d07059a12ec70deec6510b041582a88e7)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Format the Wagtail codebase with black
5 participants