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

fix and make common python version normalization #385

Merged
merged 4 commits into from
Jun 2, 2022

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Jun 1, 2022

relating to python-poetry/poetry#5717

there were two similar but diverging bits of code. They really ought to be the same; and the version that I've gone for is actually not quite the same as either of the originals.

Specifically, in the handling of python_version when specified with either one or two digits...

First, the single digit case.

  • I believe that a constraint like python_version > 3 is strictly not valid, PEP 508 says that the python_version should be equivalent to '.'.join(platform.python_version_tuple()[:2]).
  • However experiment with pip and a requirements.txt teaches me that in my 3.8.10 environment, python_version > 3 does allow a dependency to be installed
  • python_version <= "3" does not allow installation at 3.8.10
  • So I conclude that the interpretation of >3 as > 3.0.0 per Fix an error when handling single digit Python markers #155 is reasonable, and have removed code that treats >3 as >=4.0.0

For the two digit case though, experiment shows that python_version > "3.8" does indeed cause pip not to install a dependency in my 3.8.10 environment, that I think should be reinterpreted as >=3.9.0. So I have included that code.

@dimbleby dimbleby force-pushed the consistent-python-version branch from f554b71 to 3d1d83a Compare June 1, 2022 12:53
@dimbleby dimbleby force-pushed the consistent-python-version branch from 3d1d83a to a571fa5 Compare June 1, 2022 12:56
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

I think that being bug/implementation compatible with Pip here is, unfortunately, the correct move. That being said, I would like to put a streamlined version of your testing/research in the docstring or comments as appropriate. Looks good to merge after that.

Also handle a single digit of precision more carefully at the boundary.
@dimbleby
Copy link
Contributor Author

dimbleby commented Jun 1, 2022

I added a comment.

I also went to the trouble of installing python 3.0.0, to see how single digit precision really works at the edge cases.

Turns out that python_version <= "3" is not satisfied, according to pip, at 3.0.0. I reckon that probably what's happening is that the strings "3" (from the marker) and "3.0" (from the actual python version) are compared, as strings.

So I've added a comment saying that, and updated the code accordingly (not that there's anyone out there running 2.0.0 or 3.0.0 who will ever care about the difference)

@dimbleby
Copy link
Contributor Author

dimbleby commented Jun 1, 2022

Hmm, not sure I appreciate sonarcloud telling me that it doesn't like this code, let's try a small rearrangement...

@dimbleby dimbleby force-pushed the consistent-python-version branch 6 times, most recently from fcb9446 to f4e3732 Compare June 1, 2022 21:49
@dimbleby dimbleby force-pushed the consistent-python-version branch from f4e3732 to 8207439 Compare June 1, 2022 21:59
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dimbleby
Copy link
Contributor Author

dimbleby commented Jun 1, 2022

Good luck to anyone who thinks they can make this function satisfy sonarcloud, I've suppressed the warning

@neersighted
Copy link
Member

Hmm, not sure I appreciate sonarcloud telling me that it doesn't like this code, let's try a small rearrangement...

Honestly I find the SonarCloud-driven version to be more readable and cleaner :)

LGTM if suppressing Sonar is all we can do, despite the mild refactor.

@neersighted neersighted merged commit dd96ae7 into python-poetry:main Jun 2, 2022
@dimbleby dimbleby deleted the consistent-python-version branch June 2, 2022 11:06
@radoering radoering mentioned this pull request Jul 9, 2022
bostonrwalker pushed a commit to bostonrwalker/poetry-core that referenced this pull request Aug 29, 2022
Also handle a single digit of precision more carefully at the boundary.
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