Use black code formatter #3852

krzysztofwolski · 2019-03-19T22:55:10Z

I want to merge this change because... ref #3846

Screenshots

All done! 💥 💔 💥
644 files would be reformatted, 89 files would be left unchanged.

Todo

Pull Request Checklist

Privileged views and APIs are guarded by proper permission checks.
All visible strings are translated with proper context.
All data-formatting is locale-aware (dates, numbers, and so on).
Database queries are optimized and the number of queries is constant.
Database migration files are up to date.
The changes are tested.
The code is documented (docstrings, project documentation).
GraphQL schema and type definitions are up to date.
Changes are mentioned in the changelog.

krzysztofwolski · 2019-03-19T22:56:18Z

pyproject.toml

+[tool.black]
+target_version = ['py36', 'py37', 'py38']
+include = '\.pyi?$'
+exclude = '''


I've extended default exclude list for faster file discovery. We don't need to format cache dirs.

Is it really needed to include .pyi stub files?

codecov · 2019-03-19T23:07:53Z

Codecov Report

Merging #3852 into master will increase coverage by 0.05%.
The diff coverage is 89.08%.

@@            Coverage Diff             @@
##           master    #3852      +/-   ##
==========================================
+ Coverage   91.56%   91.62%   +0.05%     
==========================================
  Files         277      277              
  Lines       15106    15108       +2     
  Branches     1474     1474              
==========================================
+ Hits        13832    13842      +10     
+ Misses        877      868       -9     
- Partials      397      398       +1

Impacted Files	Coverage Δ
saleor/graphql/core/filters.py	`86.66% <ø> (ø)`	⬆️
saleor/core/utils/text.py	`100% <ø> (ø)`	⬆️
saleor/graphql/discount/resolvers.py	`100% <ø> (ø)`	⬆️
saleor/account/validators.py	`100% <ø> (ø)`	⬆️
saleor/graphql/order/schema.py	`100% <ø> (ø)`	⬆️
saleor/graphql/product/scalars.py	`52.63% <ø> (ø)`	⬆️
saleor/checkout/urls.py	`100% <ø> (ø)`	⬆️
saleor/dashboard/taxes/forms.py	`100% <ø> (ø)`	⬆️
saleor/dashboard/customer/urls.py	`100% <ø> (ø)`	⬆️
saleor/account/urls.py	`100% <ø> (ø)`	⬆️
... and 322 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3959599...c1d952a. Read the comment docs.

saleor/checkout/__init__.py

Pipfile

saleor/account/forms.py

tests/api/conftest.py

maarcingebala · 2019-03-20T09:05:07Z

With this PR we're adding .toml file which is yet another configuration Python file in our repo. Since .toml files are supposed to become a standard in Python, it would be nice to migrate to toml entirely. This is obviously beyond the scope of this PR but it's something to consider. I don't know if pipenv supports toml, maybe having only one configuration file would be only possible if we migrated to Poetry.

jxltom · 2019-03-20T09:13:58Z

Pipfile is actually toml format, but pipenv iteself doesn't support pyproject.toml yet.

krzysztofwolski · 2019-03-20T10:09:53Z

Depending on psf/black#759
Without that black produces code unsupported by py3.5

jxltom · 2019-03-20T10:21:22Z

Plus, psf/black#763 should fix this issue.

maarcingebala · 2019-03-26T13:47:54Z

We have to put this PR on hold until the next version of Black is released. This is supposed to happen in April (no exact date yet).

jxltom · 2019-03-26T13:58:22Z

As psf/black#763 is merged, and Pipenv supports pin any package via git commit, if we don't want to wait for black's release, black = {git = "https://github.com/ambv/black@cea13f4", editable = true} can be used in Pipfile for patched black version.

maarcingebala · 2019-03-26T14:24:21Z

@jxltom It would be great to migrate to Black as soon as possible. What's your opinion on relying on an unreleased version of a library @patrys @krzysztofwolski ?

patrys · 2019-03-26T19:11:32Z

I'd rather wait for it to be released as I fear people would still use released versions and we'd end up changing the formats back and forth.

jxltom · 2019-03-27T01:30:39Z

It won't be a problem if users install packages via our requirements_dev.txtby pip install -r requirements_dev.txt or Pipfile.lock by pipenv install --dev since Pipenv has already locked the version of black.

Besides, I waited for a released version of black for half a year via psf/black#517, but there still has no release. Hope it can be released in early April.

krzysztofwolski · 2019-03-27T10:03:15Z

I would rather not use an unreleased version of the software which calls itself This is a beta product ;)
I agree with @patrys

NyanKiyoshi

Note that if we change the line length, we have to update .editorconfig

krzysztofwolski · 2019-04-08T11:18:54Z

re migration to toml: our tools slowly migrate to "new" format https://gitlab.com/pycqa/flake8/issues/428 and toml libs seem to be an issue. Shame it's not JSON.

maarcingebala · 2019-04-30T11:16:14Z

I just realized that in PR #4028 we're removing support for Python 3.5 which means we'll be no longer blocked by the issue in Black 🎉 .

krzysztofwolski · 2019-05-06T13:45:23Z

@maarcingebala this week I'll update PR 🎉

NyanKiyoshi · 2019-05-08T13:02:22Z

Exclude C0330 from pylint checks
Pylint produces false positives for C0330 which makes it harder to pass the codeclimate checks with black. In anyway, black is supposed to detect any bad continuation. Thus replacing C0330.

Refering to pylint-dev/pylint#289 (open since 2014).

NyanKiyoshi · 2019-05-08T14:25:39Z

I updated the docs to mention pre-commit & black and added a black badge to the readme.

docs/contributing/coding-style.rst

Pylint produces false positives for C0330 which makes it harder to pass the codeclimate checks with black. In anyway, black is supposed to detect any bad continuation. Thus replacing C0330.

krzysztofwolski added the in progress label Mar 19, 2019

krzysztofwolski requested a review from maarcingebala March 19, 2019 22:55

krzysztofwolski commented Mar 19, 2019

View reviewed changes