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

Merge python_version and python_full_version markers #388

Merged

Conversation

radoering
Copy link
Member

Relates-to: python-poetry/poetry#5717

  • Added tests for changed code.
  • Updated documentation for changed code.

Reintroduces merging of python_version and python_full_version, which was removed in #382 because of python-poetry/poetry#5717

The constraint of the python_marker is converted via get_python_constraint_from_marker() and thus should be consistent with the latest findings described in #385.

@radoering radoering force-pushed the merge-python-version-markers branch from 69a8b76 to ed6418b Compare June 2, 2022 19:38
@radoering radoering requested a review from a team June 2, 2022 20:02
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.

Given how brittle this has been I'm a little nervous about your change as I'm not quite familiar enough with the subtleties here -- however the set of test cases look quite robust and I can easily reason about that part of the change.

@dimbleby could you PTAL?

src/poetry/core/version/markers.py Outdated Show resolved Hide resolved
src/poetry/core/version/markers.py Outdated Show resolved Hide resolved
src/poetry/core/version/markers.py Outdated Show resolved Hide resolved
@dimbleby
Copy link
Contributor

dimbleby commented Jun 2, 2022

I think this looks correct, I'll leave a couple of style quibbles.

I continue to have a slight feeling of unease that this code has become so elaborate: I like a marker simplification as much as the next person, but there's so much of it!

src/poetry/core/version/markers.py Outdated Show resolved Hide resolved
src/poetry/core/version/markers.py Outdated Show resolved Hide resolved
src/poetry/core/version/markers.py Outdated Show resolved Hide resolved
@neersighted
Copy link
Member

I think this looks correct, I'll leave a couple of style quibbles.

I continue to have a slight feeling of unease that this code has become so elaborate: I like a marker simplification as much as the next person, but there's so much of it!

I don't necessarily disagree, but I think ultimately we get higher quality results if we try to make sure the markers are as consistent and clean as possible. I think our best tool for maintainability is robust and through testing, and incremental refactoring as better patterns make themselves apparent. That being said, yes there's getting to be a lot of it and if I don't pay attention for a few weeks I forget how it works again 😆

@radoering radoering force-pushed the merge-python-version-markers branch from ed6418b to 38a03a1 Compare June 3, 2022 04:46
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 3, 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
1.2% 1.2% Duplication

@radoering
Copy link
Member Author

I continue to have a slight feeling of unease that this code has become so elaborate: I like a marker simplification as much as the next person, but there's so much of it!

It's just necessary for the solver to not try solving some bogus overrides. Solving not just for one environment is complex, but that's one of poetry's USPs.

@neersighted neersighted merged commit 077f5e9 into python-poetry:main Jun 3, 2022
@radoering radoering mentioned this pull request Jul 9, 2022
@radoering radoering deleted the merge-python-version-markers branch November 24, 2024 12:41
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.

3 participants