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

Migrate to Black #3846

Closed
maarcingebala opened this issue Mar 18, 2019 · 8 comments
Closed

Migrate to Black #3846

maarcingebala opened this issue Mar 18, 2019 · 8 comments

Comments

@maarcingebala
Copy link
Member

Currently, in Saleor we use Yapf code formatter for Python. The problem is that we've never actually formatted the whole codebase with it which results in mixed code styles across the project. Another problem is that we don't have any automatic formatting enabled so we cannot enforce a unified style in PRs from the community.

We could stick to Yapf, but in the meantime, a new tool appeared - Black - and it's becoming adopted by many other Python projects. A few teams also use it at Mirumee and they recommend it. After internal discussion, we decided to migrate to Black in Saleor as well. This change will affect every open PR and will probably cause a lot of merge conflicts for those how regularly update Saleor, but this step is necessary to improve the readability and consistency of Saleor's codebase.

What needs to be done:

  • add black to dev requirements
  • apply the new formatting
  • configure automatic formatting - I'd like to hear some feedback on that as I don't know what might be the best way to achieve that. One solution that I found is pre-commit and it looks quite promising.
@maarcingebala maarcingebala pinned this issue Mar 18, 2019
@jxltom
Copy link
Contributor

jxltom commented Mar 18, 2019

Maybe we could also consider adding isort/db-migration/graphql-schema/ts-types checks in pre-commit.

We may need following part in Pipfile since black still doesn't have a stable release.

[pipenv]
allow_prereleases = true

@maarcingebala
Copy link
Member Author

For now, I would rather only one tool and if it works for us, reconsider adding automating more in the future.

@krzysztofwolski
Copy link
Member

Also, there should be proper tests launched on CI.

Re isort - I like this tool, but sometimes I've got problems when using with black. And pre-commit can't work with it - theres issue with python environment separation.

@krzysztofwolski
Copy link
Member

@jxltom thanks for code review! 💪
As you noticed, black has a nonstandard line length of 88 chars. Personally, I like the reasoning and approach of its authors: https://github.com/ambv/black#line-length
88 Chars give extra breathing room when using types and clear (I mean long 😆) variable names. And still can open 2 files on one screen without horizontal scrolling.

Do we embrace 88?

@krzysztofwolski
Copy link
Member

Update: We are waiting for version 1.0 - ATM black is preparing new version which will for sure affect our codebase. To Avoid making massive formatting twice, we will wait.

Related tweet: https://twitter.com/llanga/status/1106247623802060802

@NyanKiyoshi
Copy link
Member

About pre-commit, I would suggest to run it inside a GH-action. It would be much faster and won't fail the travis build.

Unless there is a better solution (like a GH bot or something, that could generate rapports).

@NyanKiyoshi
Copy link
Member

Note that we also need to update the docs accordingly.

@maarcingebala
Copy link
Member Author

Merged #3852. A new era has begun 😄

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

No branches or pull requests

4 participants