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 link_hash back into _hashes #11696

Merged
merged 1 commit into from
Jan 8, 2023
Merged

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Jan 4, 2023

I’m not sure how to describe this, but my instinct is trying to tell me something’s wrong.

I was investigating #11692 and suddenly realised that Link currently has a totally unused attribute _hashes. This was introduced quite a while ago, and was (as the name suggests) used to check whether the linked file is valid. But in bad03ef, a new attribute link_hash was introduced to do the same thing (? not entirely sure) but more limited (only one hash value) and removed all calls to _hashes. However, there are still code that populates the entirely unused _hashes.

Since the plural variant covers more use cases (a file can be hashed with multiple algorithms), I restored the old logic that uses _hashes before the commit, and consolidate link_hash back into that attribute.

cc @cosmicexplorer as the person that introduced link_hash.

@uranusjr uranusjr added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jan 4, 2023
@uranusjr uranusjr requested a review from dstufft January 4, 2023 10:24
Commit bad03ef introduced the new
link_hash attribute that holds the link's hash info, but that attribute
does the same thing as _hashes, and some existing usages still populate
that old attribute. Since the plural variant covers more use cases (a
file can be hashed with multiple algorithms), we restore the old logic
that uses _hashes before the commit, and consolidate link_hash back into
that attribute.
@uranusjr uranusjr marked this pull request as ready for review January 4, 2023 11:24
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

This fixes #11692 and #11527 as well as #11557, and looks good to me, so 👍

@sbidoul
Copy link
Member

sbidoul commented Jan 8, 2023

@uranusjr perhaps you could cherry-pick the new test from #11538 ?

@sbidoul sbidoul added this to the 23.0 milestone Jan 8, 2023
@pradyunsg pradyunsg merged commit 3b60e36 into pypa:main Jan 8, 2023
@uranusjr uranusjr deleted the fix-link-hashes branch January 9, 2023 02:04
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants