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

don't merge python_version and python_full_version #382

Merged
merged 1 commit into from
May 30, 2022

Conversation

dimbleby
Copy link
Contributor

These markers are not quite equivalent; I've updated the relevant testcases to give examples where the previous code got it wrong.

cf python-poetry/poetry#5717

I do not claim that this is the only code that confuses python_version and python_full_version, I just remember this particular example because I wrote it myself!

These markers are not quite equivalent
@sonarcloud
Copy link

sonarcloud bot commented May 29, 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
No Duplication information No Duplication information

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.

LGTM, but I'm curious how we got here -- did everyone just conflate the two of these together and not realize the subtle difference (that python_version is basically always .0, if I understand correctly)? Can we add some documentation somewhere that explicitly defines the intention for each of these (or link to the upstream docs)?

@dimbleby
Copy link
Contributor Author

python_version is basically always .0 ...

that's not wrong exactly, but thinking that way is pretty much the misconception that I had when introducing the code that this MR is now removing.

python_version x.y and python_full_version x.y.0 are not interchangeable. Eg python_version > 3.6 is more like python_full_version >= 3.7.0 than it is like python_full_version > 3.6.0.

@neersighted
Copy link
Member

python_version is basically always .0 ...

that's not wrong exactly, but thinking that way is pretty much the misconception that I had when introducing the code that this MR is now removing.

python_version x.y and python_full_version x.y.0 are not interchangeable. Eg python_version > 3.6 is more like python_full_version >= 3.7.0 than it is like python_full_version > 3.6.0.

See, that's definitely needed somewhere in this codebase... Any thoughts on a good place to document it or link to docs?

@dimbleby
Copy link
Contributor Author

I don't know where would be a good place to put this. Use of these markers is scattered around the codebases: wherever one wrote this down, I'd think there's a good chance that the next person wouldn't happen to find it.

Open to suggestions!

Might be better to continue this part of the discussion in python-poetry/poetry#5717 which I intended to cover the general issue, this MR is only a specific example of getting it wrong.

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