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

Use black for formatting the code #887

Merged
merged 4 commits into from
Nov 17, 2020
Merged

Conversation

tevansuk
Copy link
Contributor

Use black to format the code

  • Add black configuration
  • Add flake8-black to check black formatting during CI
  • Update isort configuration to be black compatible
  • Add pre-commit to run black, isort, flake8, AST checks on commit
  • Add .editorconfig to fix inconsistent indentation in tox.ini/setup.cfg

I know this can be a little subjective; some people aren't keen on black formatting, and it's tedious to review..

I really did this because I'm looking at the failed merge of OpenID Connect support, and a lot of the changes in #859 appear to not be related to the OIDC support, but seem to be black autoformatting, so this will be useful for me to pull out the actual diff from that PR, even if this isn't accepted.

One bone of contention might be on the imports. Previously, isort was set to a lower line length limit than flake8 was; black cannot understand this, so the line-length has to be the same in black/flake8/isort. This has altered some of the import lines a bit.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

Current line length histogram, for information

~/bin/line-length-hist.awk $(find oauth2_provider tests -name '*.py' ! -path '*/migrations/*')

  0-4     ************************************************************************************ 1680
  5-9     *********************** 443
 10-14    ************************** 520
 15-19    ******** 145
 20-24    ***************** 337
 25-29    ******************** 391
 30-34    ****************************** 585
 35-39    ******************************* 616
 40-44    ***************************** 562
 45-49    **************************** 547
 50-54    ************************************ 706
 55-59    ********************* 410
 60-64    ******************** 387
 65-69    ********************* 403
 70-74    ************** 273
 75-79    *********** 219
 80-84    ******** 149
 85-89    ******* 138
 90-94    ******* 136
 95-99    **** 63
100-104   *** 56
105-109   * 20
110-114   *** 52
#!/usr/bin/awk -f

function ceil(valor) { return (valor == int(valor)) ? valor : int(valor)+1 }

{ ++buckets[int(length()/5)] }
END {
    for (i in buckets) {
        b = i * 5
        printf "%3d-%-3d   ", b, b+4
        print gensub(/ /, "*", "g", sprintf("%*s", ceil(buckets[i]/20), "")), buckets[i] 
    }
}

@coveralls
Copy link

coveralls commented Oct 12, 2020

Pull Request Test Coverage Report for Build 1503

  • 56 of 58 (96.55%) changed or added relevant lines in 15 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 94.821%

Changes Missing Coverage Covered Lines Changed/Added Lines %
oauth2_provider/http.py 0 1 0.0%
oauth2_provider/models.py 12 13 92.31%
Files with Coverage Reduction New Missed Lines %
oauth2_provider/oauth2_validators.py 1 91.03%
Totals Coverage Status
Change from base Build 1502: 0.0%
Covered Lines: 1300
Relevant Lines: 1371

💛 - Coveralls

@MattBlack85
Copy link
Contributor

@tevansuk personally I am a big fan of black, I don't mean I like all the styles it applies but the fact that I don't have to worry about code formatting is a big thing to me. Will mark this as a potential 1.4.0 patch waiting for others to chime in a say what they think about it

@MattBlack85 MattBlack85 added this to the 1.4.0 milestone Oct 16, 2020
@smithdc1
Copy link
Contributor

I am +1 for black, I also like the addition of a pre-commit hook. I find this very helpful. A couple of small comments.

I see you've added flake8-black. What are the benefits of this over the inbuilt check functionality of black?
What was the reason behind 110 line length, if went with default could this remove the need for the pyproject.toml file?

@tevansuk
Copy link
Contributor Author

I am +1 for black, I also like the addition of a pre-commit hook. I find this very helpful. A couple of small comments.

Thanks for the review :)

I see you've added flake8-black. What are the benefits of this over the inbuilt check functionality of black?

flake8-black integrates black linting when flake8 is run during CI, so if a developer has not installed the recommended pre-commit hooks, and has allowed some code that violates black to be committed, it would get flagged during the current CI checks. It allows us to integrate black checking without adding a new CI step.

What was the reason behind 110 line length, if went with default could this remove the need for the pyproject.toml file?

This just matches the current maximum line length specified in flake8's configuration. If we kept the default line length then we wouldn't need pyproject.toml, but the diff would be significantly larger - at least 175 new changes. Line length changes are always contentious - its a "bike shed problem", everyone can have an opinion on what colour it should be painted :)

On my own projects I always change the line length because the default number in black is (accidentally) the same as a racist hate symbol.

PS: I probably won't keep chasing HEAD on this, but ping me any time you'd like this PR updated 👍

@MattBlack85
Copy link
Contributor

@tevansuk I think this is good to go, do you mind updating your branch? I am gonna merge it after that

@tevansuk
Copy link
Contributor Author

@tevansuk I think this is good to go, do you mind updating your branch? I am gonna merge it after that

Sounds good 👍 - I'll start updating it now

* Add black configuration
* Run black as part of flake8 testenv in tox
* Add editorconfig to ensure indent style in tox.ini/setup.cfg
* Add pre-commit hooks to check flake8, black, isort and common errors
* Update isort configuration to be black-compatible
* Update contributing documentation
* Add myself to AUTHORS
* Skip migrations in black/isort/pre-commit
This is the result of running `black .` over the repository.

By-hand improvements of the blackened code will be in follow up commits,
to make it easier to reapply this commit to future updates, if
necessary - IE to remove this commit and re-run black over a fresh tree,
rather than trying to merge new changes in to this commit.
Some minor hand tweaks:

    oauth2_provider/contrib/rest_framework/authentication.py
    oauth2_provider/oauth2_validators.py
        Construct OrderedDict in a clearer, still black compliant way
        (one line per dict entry)

    tests/test_token_revocation.py
        Remove empty method docstrings
@MattBlack85 MattBlack85 merged commit 5cb5398 into jazzband:master Nov 17, 2020
@n2ygk n2ygk modified the milestones: 1.5.0, 1.4.0 Feb 8, 2021
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.

None yet

5 participants