Skip to content

Use Black and isort for code formatting #368

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

Merged
merged 1 commit into from
Jul 4, 2021
Merged

Use Black and isort for code formatting #368

merged 1 commit into from
Jul 4, 2021

Conversation

sisp
Copy link
Contributor

@sisp sisp commented Jul 3, 2021

This PR adds Black and isort for code formatting. It addresses the second and fourth suggestion of #355. Since pre-commit is already being used, I thought it made sense to integrate the formatters already in this PR.

Some notes about this PR:

  • I've added Black and isort as dev-dependencies and set up pre-commit hooks that use those local executables instead of pulling them from remote repos. The reason is that when developing with a proper IDE like VS Code that supports those code formatters, then both the IDE and the pre-commit hooks (and thus also the CI job running static checks) should use the same versions of those formatters. Also, in order for isort to detect first and third party packages correctly, it needs to be installed in the same virtual environment as all the other packages.
  • I've configured Black and isort to use a line length of 79 characters, so that those two and Flake8 are in agreement. Black uses a line length of 88 characters by default, so if you would like to stick with Black's default setting, I can change the configuration.
  • I've configured isort to use a separate line for each import. Several high-quality projects are using this formatting style, too, e.g. TensorFlow and Poetry. This formatting style makes diffs easier to read and - at least to me - is visually more appealing. Let me know if you're happy with it or whether you would like it changed back to having several imports from the same package/module per line.
  • The GitHub Actions workflows are getting a bit redundant as the code for the setup of Poetry and installation of packages is copied several times, one more time now because pre-commit requires Black and isort to be installed (as mentioned above), so the job that runs code quality checks also performs those steps now. I think there is an opportunity for refactoring the workflows, but I suggest to create a dedicated PR for it. Do you agree?

@codecov
Copy link

codecov bot commented Jul 3, 2021

Codecov Report

Merging #368 (f6415ed) into master (b65c972) will increase coverage by 2.27%.
The diff coverage is 96.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
+ Coverage   70.95%   73.23%   +2.27%     
==========================================
  Files          80       80              
  Lines        2355     2391      +36     
==========================================
+ Hits         1671     1751      +80     
+ Misses        684      640      -44     
Impacted Files Coverage Δ
openapi_core/casting/schemas/exceptions.py 100.00% <ø> (ø)
openapi_core/contrib/django/__init__.py 83.33% <ø> (ø)
openapi_core/contrib/django/responses.py 92.85% <ø> (ø)
openapi_core/contrib/falcon/__init__.py 100.00% <ø> (ø)
openapi_core/contrib/flask/__init__.py 83.33% <ø> (ø)
openapi_core/contrib/flask/decorators.py 55.88% <ø> (ø)
openapi_core/contrib/flask/providers.py 100.00% <ø> (ø)
openapi_core/contrib/flask/responses.py 87.50% <ø> (ø)
openapi_core/contrib/requests/__init__.py 83.33% <ø> (ø)
...pi_core/deserializing/media_types/deserializers.py 95.00% <ø> (ø)
... and 100 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 b65c972...f6415ed. Read the comment docs.

@p1c2u
Copy link
Collaborator

p1c2u commented Jul 4, 2021

I've configured isort to use a separate line for each import. Several high-quality projects are using this formatting style, too, e.g. TensorFlow and Poetry. This formatting style makes diffs easier to read and - at least to me - is visually more appealing. Let me know if you're happy with it or whether you would like it changed back to having several imports from the same package/module per line.

That's interesting I never saw this before bit more important for me is to stick with one rule. Let's try it then.

Copy link
Collaborator

@p1c2u p1c2u left a comment

Choose a reason for hiding this comment

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

it's hard to review 122 files so I assume it's just formatting change 😂

@p1c2u p1c2u merged commit 4ce032c into python-openapi:master Jul 4, 2021
@sisp
Copy link
Contributor Author

sisp commented Jul 4, 2021

Yes, it is. The only manual change I made was remove some pass statements in exception classes that have a class docstring, so pass is not needed in that case.

@sisp sisp deleted the code-formatters branch July 4, 2021 14:30
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.

2 participants