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

Refs #33476 -- Reformatted code with Black. #15387

Merged
merged 5 commits into from Feb 8, 2022

Conversation

carltongibson
Copy link
Member

@carltongibson carltongibson commented Feb 2, 2022

Mainly targeting coding-style.txt, but then spread to a few of the config files.

  • Not sure they're exactly how we want them?
  • Doesn't yet include the GitHub Action, which wouldn't pass until we actually run black.
  • Skipping pre-commit until we're ready to run it...

@carltongibson carltongibson requested a review from adamchainz Feb 2, 2022
@carltongibson
Copy link
Member Author

@carltongibson carltongibson commented Feb 2, 2022

Need to bump isort too? 🧐

isort.exceptions.ProfileDoesNotExist: Specified profile of "black" does not exist. 
Available profiles: black,django,pycharm,google,open_stack,plone,attrs,hug,wemake,appnexus.

Computers! 😄 "Specified profile of "black" does not exist." vs "Available profiles: black,..."

flake8 fails too because of the line length. (May as well add the GHA...)

@carltongibson carltongibson marked this pull request as draft Feb 2, 2022
setup.cfg Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@carltongibson
Copy link
Member Author

@carltongibson carltongibson commented Feb 3, 2022

isort failure is resolved (stably) (by the look of it) after running black and then isort again.

@felixxm as per discussion, you'll add the commit running black here, then we can ask the @django/technical-board to approve, and relevant commits to .git-blame-ignore-revs to finished off.

@carltongibson carltongibson force-pushed the tweaks-for-black branch 2 times, most recently from a019215 to e995494 Compare Feb 3, 2022
@felixxm felixxm changed the title Refs #33476 -- Adjusted docs and config files for Black. Refs #33476 -- Reformatted code with Black. Feb 3, 2022
@felixxm
Copy link
Member

@felixxm felixxm commented Feb 4, 2022

view_tests.tests.test_debug.DebugViewTests.test_template_exceptions started crashing after reformatting the code with Black, it should not happen 😱 💣

Traceback (most recent call last):
  File "tests/view_tests/tests/test_debug.py", line 287, in test_template_exceptions
    self.assertNotEqual(
AssertionError: -1 == -1 : Failed to find 'raise Exception' in last frame of traceback, instead found: raise Exception("boom")

I fixed it in a separate commit.

@felixxm felixxm force-pushed the tweaks-for-black branch 2 times, most recently from 13030e6 to 2f9a20f Compare Feb 4, 2022
@carltongibson
Copy link
Member Author

@carltongibson carltongibson commented Feb 4, 2022

@felixxm It looks like the flake8 setting is too strict at 88 chars...

@felixxm
Copy link
Member

@felixxm felixxm commented Feb 4, 2022

@felixxm It looks like the flake8 setting is too strict at 88 chars...

I'm working on updating these ~2000 lines (will do this in a separate commit). IMO we want to be strict, as we were in the past.

@adamchainz
Copy link
Sponsor Member

@adamchainz adamchainz commented Feb 4, 2022

Running once with black --preview will rewrap long strings

@felixxm
Copy link
Member

@felixxm felixxm commented Feb 4, 2022

Running once with black --preview will rewrap long strings

Unfortunately, it doesn't fix all cases and it introduces other unnecessary changes 😞

setup.cfg Show resolved Hide resolved
@felixxm felixxm force-pushed the tweaks-for-black branch 4 times, most recently from ac7de90 to 92ef633 Compare Feb 4, 2022
@felixxm
Copy link
Member

@felixxm felixxm commented Feb 7, 2022

I added release notes and versionchanged annotation.

@felixxm felixxm marked this pull request as ready for review Feb 7, 2022
@felixxm
Copy link
Member

@felixxm felixxm commented Feb 7, 2022

@django/technical-board This is a DEP-0008's implementation milestone, so according to DEP-0010 I'd like to inform the Technical Board of an intent to merge it. Please hold a vote 🗳️ Voting will end on February 14th, 2022 📆

This PR is almost impossible to rebase, so we'd like to merge it when all members of the Technical Board have cast their vote without waiting to February 14th, 2022. If any of the members of the board have something against it, please express it when casting your vote.

As a follow up, we will create .git-blame-ignore-revs with the following commits:

  • Refs #33476 -- Used vertical hanging indentation for format lists with inline comments.
  • Refs #33476 -- Refactored problematic code before reformatting by Black.
  • Refs #33476 -- Reformatted code with Black.
  • Refs #33476 -- Refactored code to strictly match 88 characters line length.

@apollo13
Copy link
Member

@apollo13 apollo13 commented Feb 7, 2022

This is my +1 in my capacity as a current technical board member. :shipit:

Thank you all so much for the work on this, even if we don't agree on all points.

@apollo13
Copy link
Member

@apollo13 apollo13 commented Feb 7, 2022

Also: https://www.youtube.com/watch?v=O4irXQhgMqg

@andrewgodwin
Copy link
Member

@andrewgodwin andrewgodwin commented Feb 7, 2022

+1 from me on merging. Thanks for working on this!

@charettes
Copy link
Member

@charettes charettes commented Feb 7, 2022

+1 from me as well 🖤

@orf
Copy link
Contributor

@orf orf commented Feb 7, 2022

+1 from me of course

@adamchainz
Copy link
Sponsor Member

@adamchainz adamchainz commented Feb 7, 2022

+1 from me! 🖤

One small request - could you run the non-Black'd test suite once against the Black'd code? Even just locally with SQLite. I know Black checks for AST equivalence, but there's so much code changing here it's not reviewable, and it's not impossible we'd hit a bug...

@apollo13
Copy link
Member

@apollo13 apollo13 commented Feb 7, 2022

One small request - could you run the non-Black'd test suite once against the Black'd code? Even just locally with SQLite. I know Black checks for AST equivalence, but there's so much code changing here it's not reviewable, and it's not impossible we'd hit a bug...

Good point, I get no failures when running on sqlite:

Ran 15566 tests in 154.141s

OK (skipped=1152, expected failures=5)

@felixxm felixxm merged commit 7119f40 into django:main Feb 8, 2022
27 checks passed
@felixxm
Copy link
Member

@felixxm felixxm commented Feb 8, 2022

Thanks y'all 🥇

@carltongibson carltongibson deleted the tweaks-for-black branch Feb 8, 2022
@richardARPANET
Copy link

@richardARPANET richardARPANET commented Feb 8, 2022

Double quotes look terrible, try https://github.com/odwyersoftware/brunette for single quotes.

@adamchainz
Copy link
Sponsor Member

@adamchainz adamchainz commented Feb 8, 2022

@richardARPANET You won't make many internet friends by making silly drive-by comments like that.

@arthurhenrique
Copy link

@arthurhenrique arthurhenrique commented Feb 8, 2022

nice bomba patch 💣

@SimeonAleksov
Copy link

@SimeonAleksov SimeonAleksov commented Feb 16, 2022

@richardARPANET You won't make many internet friends by making silly drive-by comments like that.

Hmm, can I ask why is that silly(excluding the library that they recommended)?

I might be wrong but I remember reading something within the lines that Black maintainer said he wished/preferred to enforce single quotes.

@adamchainz
Copy link
Sponsor Member

@adamchainz adamchainz commented Feb 16, 2022

The discussion for reformatting Django with Black was had literally years ago, and then voted on again in this PR. Popping in after the fact to espouse an opinion isn't useful for anyone.

@SimeonAleksov
Copy link

@SimeonAleksov SimeonAleksov commented Feb 16, 2022

The discussion for reformatting Django with Black was had literally years ago, and then voted on again in this PR. Popping in after the fact to espouse an opinion isn't useful for anyone.

Fair enough, another suggestion is to add skip-string-normalization=true to the black config, which would make this PR way easier for reviewing.

@felixxm
Copy link
Member

@felixxm felixxm commented Feb 16, 2022

We agreed in DEP-0008 to use Black's default settings, that is, 88 characters per line and double quotes.

Locking discussions in this PR.

@django django locked as resolved and limited conversation to collaborators Feb 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet