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

Fixing deprecating distutils (PEP 632) #1730

Merged
merged 3 commits into from
Nov 25, 2021
Merged

Conversation

chayim
Copy link
Contributor

@chayim chayim commented Nov 21, 2021

Pull Request check-list

Please make sure to review and check all of these items:

  • 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)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

fixes #1729

@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2021

Codecov Report

Merging #1730 (1dbade7) into master (f4519f3) will increase coverage by 28.46%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1730       +/-   ##
===========================================
+ Coverage   62.34%   90.80%   +28.46%     
===========================================
  Files          59       59               
  Lines       14249    14249               
===========================================
+ Hits         8883    12939     +4056     
+ Misses       5366     1310     -4056     
Impacted Files Coverage Δ
setup.py 0.00% <ø> (ø)
redis/connection.py 71.89% <100.00%> (+1.42%) ⬆️
tests/test_cluster.py 98.60% <0.00%> (ø)
redis/commands/sentinel.py 64.28% <0.00%> (+10.71%) ⬆️
tests/test_pubsub.py 97.68% <0.00%> (+11.34%) ⬆️
redis/commands/core.py 84.00% <0.00%> (+11.40%) ⬆️
tests/test_commands.py 93.47% <0.00%> (+20.70%) ⬆️
redis/client.py 83.31% <0.00%> (+20.85%) ⬆️
redis/commands/json/path.py 100.00% <0.00%> (+22.22%) ⬆️
... and 32 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 f4519f3...1dbade7. Read the comment docs.

@chayim chayim changed the title Deprecating distutils (PEP 632) by adding packaging Fixing deprecating distutils (PEP 632) Nov 21, 2021
@chayim chayim added 4.0.next bug Bug and removed 4.0.next labels Nov 21, 2021
@elderlabs
Copy link

Would there be any benefit to using setuptools version comparison implementation, as many projects already require it to install other deps, instead of adding packaging as a requirement for this sole purpose?

@chayim
Copy link
Contributor Author

chayim commented Nov 22, 2021

I thought about it - as I definitely prefer to not have any dependencies (in fact, I intend to remove the deprecation dep in the future) for this type of library. There are two issues here:

  1. PEP 632 specifically refers to packaging (find, we can ignore this)
  2. Setuptools version comparison is broken for our needs- in that it fails to parse the third period (part of PEP-0440)

Here's an example of #2

from pkg_resources import parse_version as V
v456 = V('4.5.6')
v457prev = V('4.5.7preview')
v457prev2 = V('4.5.7.preview')
v457 = V('4.5.7')

v457prev > v456
>>> True

v457 > v456
>>> True

v457prev2 > v456
>>> False

@chayim chayim merged commit 884f7ad into master Nov 25, 2021
@chayim chayim deleted the ck-distuils-deprecation branch November 25, 2021 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python 3.10 distutils Deprecation Warning
3 participants