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

PR: Remove usage of distutils.LooseVersion in favor of pkg_resources.parse_version #235

Closed
wants to merge 3 commits into from

Conversation

stonebig
Copy link
Contributor

@stonebig stonebig commented Apr 15, 2021

Fixes #234

qtpy/__init__.py Outdated
@@ -62,7 +62,11 @@
"""

from distutils.version import LooseVersion
try: # disutils is being deprecated
import pkg_resources
Copy link
Contributor

Choose a reason for hiding this comment

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

this import is useless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept a backup plan, as I'm not sure on what very old Python QtPy may be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but it still useless

qtpy/__init__.py Outdated
try: # disutils is being deprecated
import pkg_resources
from pkg_resources import parse_version as LooseVersion
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

except ImportError:

@jschueller
Copy link
Contributor

jschueller commented Apr 21, 2021

I'd rather use the new name with the old symbol than the new symbol with the old name: use distutils.version.LooseVersion as parse_version

@ccordoba12 ccordoba12 changed the title deprecate distutils.LooseVersion PR: Remove usage of distutils.LooseVersion in favor of pkg_resources.parse_version Aug 23, 2021
@ccordoba12 ccordoba12 added this to the v1.11.0 milestone Aug 23, 2021
@dalthviz dalthviz modified the milestones: v1.11.0, v2.0.0 Aug 27, 2021
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Oct 11, 2021

@stonebig Actually, pkg_resources.parse_version itself is deprecated in favor of using the modern standard packaging.version.parse (which pkg_resources.parse_version is now essentially just a legacy compatibility shim for). So I'd suggest replacing the modified lines with just from packaging.version import parse, replacing LooseVersion with parse, and adding packaging under install_requires in setup.py (its extremely lightweight, installed by setuptools anyway, and is the PyPA-standard approach).

@dalthviz
Copy link
Member

Hi @stonebig checking the option proposed by @CAM-Gerlach seems like a good idea but, just in case what do you think?
And also if you are ok with the idea, would you like to do those changes here or maybe we should create a new PR that supersedes this one using the packaging module? Let us know!

Also, just in case, what do you think @ccordoba12 ?

@ccordoba12
Copy link
Member

Also, just in case, what do you think @ccordoba12 ?

I also agree with using packaging because pkg_resources is deprecated. My only question is: is packaging a setuptools vendored dependency? If so, what's the earliest version of setuptools we can rely on that includes it?

@CAM-Gerlach
Copy link
Member

@ccordoba12 It is a setuptools-vendored dep in recent versions, but we cannot rely on the Setuptools-vendored version as it is a private, internal-only implementation detail of setuptools, varies arbitrarily between setuptools versions (some may be out of date, while some may contain breaking changes), has setuptools-specific modifications, and can change or be dropped at any time. As packaging is a core officially-supported component of the PyPA ecosystem, is depended upon by many other major projects and is a lightweight pure-Python package that's only 40 kB, it seems sensible to simply add as a proper dependency under install_requires and the Conda recipe equivalent.

@ccordoba12
Copy link
Member

it seems sensible to simply add as a proper dependency under install_requires and the Conda recipe equivalent.

Ok, I'm fine with that.

@CAM-Gerlach
Copy link
Member

@stonebig You want to do that here, or would you prefer I go ahead with it (in another PR, or, at your option, on this branch)?

@CAM-Gerlach
Copy link
Member

Just FYI, on #234 , @bgermann suggested using the underlying QVersionNumber instead, which would presumably avoid adding a dependency and be more generic than the PEP 440-specific packaging solution. @dalthviz commented that it was only available on Qt 5.6+, but since in v2.0 where this is going, we're certainly dropping support for lower than that anyway, that doesn't seem to be an objection. However, the main problem I can imagine would be the case is it is needed early on before a usable Qt binding is confirmed, so it runs into a bit of a bootstrap problem, at least if I understand the suggestion correctly.

@dalthviz
Copy link
Member

I think we can go with using the packaging solution as the suggestion at https://www.python.org/dev/peps/pep-0632/#migration-advice. By the way @CAM-Gerlach do you want to work on that?

@CAM-Gerlach
Copy link
Member

Sure, opened as PR #266

@CAM-Gerlach CAM-Gerlach removed their assignment Oct 28, 2021
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.

distutils.LooseVersion is being deprecated
5 participants