Skip to content

Smarter (and looser) link equivalency logic#10079

Merged
uranusjr merged 1 commit intopypa:mainfrom
uranusjr:new-resolver-url-equivalent-no-hash
Jul 22, 2021
Merged

Smarter (and looser) link equivalency logic#10079
uranusjr merged 1 commit intopypa:mainfrom
uranusjr:new-resolver-url-equivalent-no-hash

Conversation

@uranusjr
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment on lines +1825 to +1830
pytest.param(
False,
"#subdirectory=bar&egg=foo",
"#subdirectory=rex",
id="drop-egg-still-different",
),
Copy link
Copy Markdown
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
Copy Markdown
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?

@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
@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

Development

Successfully merging this pull request may close these issues.

Resolution conflict due to the PEP 508 requirement with superfluous #egg fragment specified in a package’s metadata

4 participants