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

Added black and isort #1734

Merged
merged 8 commits into from
Nov 30, 2021
Merged

Added black and isort #1734

merged 8 commits into from
Nov 30, 2021

Conversation

WisdomPill
Copy link
Contributor

@WisdomPill WisdomPill commented Nov 21, 2021

Fixes: #1644

Pull Request check-list

  • Does $ tox pass with this change (including linting)?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Description of change

For the moment I just added isort and black, there is a problem with black because it produces different code.
For the huge changes, I used black and isort that could be used to change the code to make it comply.

black --target-version py38 --exclude tests/test_commands.py .
isort .

Note that python3.9 is not yet supported as target in black.

@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2021

Codecov Report

Merging #1734 (3a9daf4) into master (d524709) will decrease coverage by 0.01%.
The diff coverage is 89.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1734      +/-   ##
==========================================
- Coverage   90.90%   90.89%   -0.02%     
==========================================
  Files          59       59              
  Lines       14229    14229              
==========================================
- Hits        12935    12933       -2     
- Misses       1294     1296       +2     
Impacted Files Coverage Δ
redis/commands/__init__.py 100.00% <ø> (ø)
redis/commands/core.py 84.01% <ø> (ø)
redis/commands/search/__init__.py 81.81% <ø> (ø)
redis/commands/search/querystring.py 0.00% <0.00%> (ø)
redis/utils.py 77.41% <ø> (ø)
setup.py 0.00% <0.00%> (ø)
tests/test_cluster.py 98.46% <ø> (-0.14%) ⬇️
tests/test_commands.py 93.47% <ø> (ø)
redis/commands/sentinel.py 64.28% <23.07%> (ø)
redis/connection.py 71.85% <69.23%> (ø)
... and 45 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 d524709...3a9daf4. Read the comment docs.

@WisdomPill
Copy link
Contributor Author

@chayim I would propose to merge this asap, because it changes the format of many files, hence will need rerunning black and isort

@WisdomPill WisdomPill marked this pull request as ready for review November 21, 2021 19:58
@chayim
Copy link
Contributor

chayim commented Nov 22, 2021

@WisdomPill First off amazing. thank you! There will be some delay as there's a hefty PR I'd like to merge in first (Thursday ideally). Apologies on that.

Secondly - I'd like to remove the pre-commit functionality. It doesn't really work with the current workflow, in that dependencies are declared there that can be different from the dev_requirements.txt. I don't love having a requirements.txt, dev_requirements.txt, and setup.py - but adding a fourth place is IMHO asking too much of future contributors.

What do you think?

@WisdomPill
Copy link
Contributor Author

I am also not a big fan of pre-commit, as nice as it seems to be, I find it cumbersome, it needs to be synced with ci and the rest of the toolchain (tox and so on). For me it is not a big deal to have the CI complain if the linters are not happy.

I would gladly remove it

@akx
Copy link
Contributor

akx commented Nov 29, 2021

I think black --target-version py36 would be more on point considering the package has python_requires='>=3.6':

python_requires=">=3.6",

Not sure if it will make a difference in output here, though :)

@WisdomPill
Copy link
Contributor Author

@akx thanks for pointing that out, I fixed it and rebased based on the latest changes in master

@akx akx mentioned this pull request Nov 29, 2021
5 tasks
@chayim chayim added 4.1.0 maintenance Maintenance (CI, Releases, etc) labels Nov 29, 2021
@WisdomPill
Copy link
Contributor Author

@chayim here you go! 😄

@chayim
Copy link
Contributor

chayim commented Nov 30, 2021

@WisdomPill Awesome possum. Thank you so much my friend, you're a scholar, and gentlebeing. Incoming to master.

@chayim chayim merged commit b94e230 into redis:master Nov 30, 2021
@WisdomPill WisdomPill deleted the chore/1644 branch November 30, 2021 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintenance (CI, Releases, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from flake8 to black
4 participants