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

Allow Python versions with letters in the minor version suffix #82

Merged
merged 5 commits into from
Jun 22, 2021
Merged

Conversation

ulf1
Copy link
Contributor

@ulf1 ulf1 commented Jun 7, 2021

see also #27

@LysandreJik
Copy link
Member

This would probably be simpler handled with the packaging package rather than a regex, no? https://pypi.org/project/packaging/
It's from the python packaging authority and I think it would make this more readable/more robust

@ulf1
Copy link
Contributor Author

ulf1 commented Jun 8, 2021

import packaging.version
v1 = packaging.version.parse("3.9.0+")   # `.parse` doesn't throw an exception
v2 = packaging.version.Version("3.8.0")
v1 < v2
# True  # This is wrong because type(v1)=LegacyVersion
import packaging.version
v1 = packaging.version.parse("3.8.3rc1")  
v2 = packaging.version.Version("3.8.0")
v1 < v2
# False  # Ok

In other words, versions like "3.9.0+" is not PEP 440 compliant (see https://packaging.pypa.io/en/latest/version.html). packaging.version.Version("3.9.0+") would raise an InvalidVersion exception.

import packaging.version
v1 = packaging.version.Version("3.9.0+")  # <- raises InvalidVersion
v2 = packaging.version.Version("3.8.0")
v1 < v2

@@ -34,7 +35,7 @@

_PY_VERSION: str = sys.version.split()[0]

if tuple(int(i) for i in _PY_VERSION.split(".")) < (3, 8, 0):
if packaging.version.Version(_PY_VERSION) < packaging.version.Version("3.8.0"):
Copy link
Member

Choose a reason for hiding this comment

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

With that raising an InvalidVersion error, wouldn't it just crash and make the library unusable for someone in the position of #27?

Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to check how to handle this properly with poetry returning invalid versions but their website seems to be down with ERR_SSL_PROTOCOL_ERROR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With that raising an InvalidVersion error, wouldn't it just crash and make the library unusable for someone in the position of #27?

Yes it would exclude "3.9.0+". However, the packaging package claims to implement PEP 440. And according to PEP 440 "3.9.0+" is not a valid version (due to the "+" suffix).

The previous regex proposal could process "3.9.0anything123too".

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'm trying to check how to handle this properly with poetry returning invalid versions but their website seems to be down with ERR_SSL_PROTOCOL_ERROR

yesterday pypi.org had an CDN outage no pip install)

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 could add .rstrip("+") as done in https://github.com/huggingface/huggingface_hub/pull/27/files
i.e. _PY_VERSION: str = sys.version.split()[0].rstrip("+")
It seems that @ericricky isn't active rn.

Include changes made in #27
@@ -32,9 +33,9 @@

logger = logging.getLogger(__name__)

_PY_VERSION: str = sys.version.split()[0]
_PY_VERSION: str = sys.version.split()[0].rstrip("+")
Copy link
Contributor Author

@ulf1 ulf1 Jun 20, 2021

Choose a reason for hiding this comment

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

@LysandreJik I added the proposal made by @ericricky

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Yes, LGTM!

@LysandreJik
Copy link
Member

Could you run make style at the root of your clone to fix the code quality issues?

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