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

Smarter (and looser) link equivalency logic #10079

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Jun 18, 2021

Fix #10002 (eventually).

Todo:

@uranusjr uranusjr force-pushed the new-resolver-url-equivalent-no-hash branch from 503aeda to c55d17c Compare June 18, 2021 19:29
@uranusjr uranusjr force-pushed the new-resolver-url-equivalent-no-hash branch 2 times, most recently from b0adb89 to a889a22 Compare June 22, 2021 04:12
@uranusjr uranusjr added this to the 21.2 milestone Jun 22, 2021
@uranusjr uranusjr marked this pull request as ready for review June 22, 2021 04:12
@uranusjr uranusjr force-pushed the new-resolver-url-equivalent-no-hash branch from a889a22 to a323183 Compare June 22, 2021 04:13
@uranusjr uranusjr force-pushed the new-resolver-url-equivalent-no-hash branch 6 times, most recently from 48fd6a9 to c53ed18 Compare July 1, 2021 09:17
@uranusjr uranusjr force-pushed the new-resolver-url-equivalent-no-hash branch from c53ed18 to 83d092f Compare July 12, 2021 07:08
@uranusjr
Copy link
Member Author

Still hoping someone could take a look. I detailed the logic in the docstring, a brief browse would be much appreciated.

Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I've only desk-checked, and I've not really reviewed the tests, but the logic seems good.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Implementation LGTM!

A few non-blocker comments on the tests, and one maybe-should-do-later suggestion.

tests/functional/test_new_resolver.py Show resolved Hide resolved
Comment on lines +1825 to +1830
pytest.param(
False,
"#subdirectory=bar&egg=foo",
"#subdirectory=rex",
id="drop-egg-still-different",
),
Copy link
Member

Choose a reason for hiding this comment

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

nit-ish: Add another test for the reverse of this, and move this to the top with the other drop- tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverse as in reversing subdirectory and egg, or the two values to arguments?

tests/unit/test_link.py Show resolved Hide resolved
tests/unit/test_link.py Show resolved Hide resolved
src/pip/_internal/models/link.py Show resolved Hide resolved
@uranusjr uranusjr force-pushed the new-resolver-url-equivalent-no-hash branch 2 times, most recently from 3707bf1 to c7bec9b Compare July 12, 2021 19:19
news/10002.feature.rst Outdated Show resolved Hide resolved
@uranusjr uranusjr force-pushed the new-resolver-url-equivalent-no-hash branch from c7bec9b to 2da77e9 Compare July 22, 2021 07:27
@uranusjr uranusjr merged commit 26778e9 into pypa:main Jul 22, 2021
@uranusjr uranusjr deleted the new-resolver-url-equivalent-no-hash branch July 22, 2021 07:45
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants