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 code style #640

Merged
merged 13 commits into from
Nov 7, 2022
Merged

Use black code style #640

merged 13 commits into from
Nov 7, 2022

Conversation

juhoinkinen
Copy link
Member

@juhoinkinen juhoinkinen commented Nov 4, 2022

Integrates Black code style tool to development flow and formats codebase to follow Black default style.

The main changes that Black makes when using the default style seem to be:

  • changes single quotes to double quotes (where single quotes are not necessary)
  • joins some short lines, because max. line length is 88 instead of 79
  • adds a trailing comma to the last argument of list of arguments spanning multiple lines
  • in list etc. comprehensions a newline is added after open parenthesis
  • lines are broken before a binary operator
  • adds newlines after import statement
  • adds spaces around not-simple slices, e.g. to [i : i + j]

It would be possible to skip some of the changes with custom configuration (like in branch issue609-use-black-code-style-less-changes, where quotes are not changed and the 79 max. line length is retained). However, I think it could be better just to use the default Black style.

The .git-blame-ignore-revs file hides (some) of the changed lines in git-blame view of the style migration commit.

Manual local usage

The Black tool is added to the development dependencies, and the codebase can be formatted with one command. If nothing is left to be changed, the run is:

black .
All done! ✨ 🍰 ✨
92 files left unchanged.

When using Black the autopep8.sh script is unnecessary and it is removed.

Automated local usage

The pre-commit hook for linting with black and flake8 can be configured by using the following in the file .git/hooks/pre-commit:

#!/bin/sh

black . --check --diff
flake8

There exists the precommit-framework, which could automate the installation of the hook, but the hook would need to be defined in .pre-commit-config.yaml file. I don't know if using the precommit-framework is worth adding yet another dev dependency. This blog showed how that could be used.

(There exists also the pytest-black plugin, but it seems unmaintained.)

CI usage

Adds a new "lint with Black" job to GitHub Actions CI workflow, which uses the Action provided by Black developers to just check the format (uses options --check --diff). It is run in parallel with unit test jobs.

I noticed that the separate Flake8 step for linting has been redundant, because flake8 linting is run also with the pytest, so the separate Flake8 step is removed.

IDE integration

In VS Code the Black was already included and could be selected as the formatter.

Closes #609.

@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Base: 99.58% // Head: 99.58% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (f5bc54d) compared to base (1208238).
Patch coverage: 99.53% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #640   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files          87       87           
  Lines        5990     5991    +1     
=======================================
+ Hits         5965     5966    +1     
  Misses         25       25           
Impacted Files Coverage Δ
annif/analyzer/snowball.py 100.00% <ø> (ø)
annif/corpus/combine.py 100.00% <ø> (ø)
annif/views.py 80.00% <66.66%> (ø)
annif/__init__.py 88.46% <70.00%> (ø)
annif/vocab.py 97.50% <92.85%> (ø)
annif/backend/yake.py 98.21% <96.15%> (ø)
annif/project.py 99.41% <96.55%> (ø)
annif/analyzer/__init__.py 93.93% <100.00%> (ø)
annif/analyzer/analyzer.py 100.00% <100.00%> (ø)
annif/analyzer/spacy.py 100.00% <100.00%> (ø)
... and 70 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@juhoinkinen juhoinkinen changed the title Issue609 use black code style Use black code style Nov 4, 2022
@juhoinkinen juhoinkinen added this to the 0.60 milestone Nov 4, 2022
@juhoinkinen
Copy link
Member Author

BTW, there exists also the isort tool to sort import statements, which would be one automation step more.

@juhoinkinen
Copy link
Member Author

CodeClimate complains about too long lines.

The complaints by other code-style tools seem unimportant. The SonarCloud complaints about http protocol could could be just marked safe.

@juhoinkinen juhoinkinen marked this pull request as ready for review November 4, 2022 14:36
@juhoinkinen
Copy link
Member Author

@osma How does this look? One question is whether to switch using max. line length of 88 chars.

@osma
Copy link
Member

osma commented Nov 4, 2022

I think it looks very good already.

Re: line length, I think 88 is OK, but is there a way to tell that to Code Climate so it won't complain?

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

Very good!

@sonarcloud
Copy link

sonarcloud bot commented Nov 7, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 98 Security Hotspots
Code Smell A 70 Code Smells

No Coverage information No Coverage information
2.4% 2.4% Duplication

@juhoinkinen
Copy link
Member Author

Force pushed after fixing completely typoed commit message.

The commit to README.md fixed my misunderstanding of the pre-commit hook which was recommended: the hook should only check whether style is good, not fix it in the same go (it could fix it, but the fixed code would not end up in the commit but only in the working tree, as it would not be staged).

@juhoinkinen juhoinkinen merged commit 7f2ce22 into master Nov 7, 2022
@juhoinkinen juhoinkinen deleted the issue609-use-black-code-style branch November 7, 2022 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Black code style?
2 participants