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

Do not depend on cached download for copying downloaded file #7474

Merged
merged 2 commits into from
Dec 13, 2019

Conversation

chrahunt
Copy link
Member

@chrahunt chrahunt commented Dec 13, 2019

Previously we depended on the result of the check for a cached download in order to decide whether we would copy the downloaded file. Now we do not. This will allow us to eventually extract the copying of the downloaded file out of unpack_file_url and unpack_http_url.

_check_download_dir will only return a falsy value if either:

* the provided path does not exist
* the hash does not match - in which case the file is unlinked

so the file cannot exist at either of these points.
@chrahunt chrahunt added type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Dec 13, 2019
@chrahunt chrahunt changed the title Refactor/operations prepare Do not depend on cached download for copying downloaded file Dec 13, 2019
@chrahunt chrahunt marked this pull request as ready for review December 13, 2019 04:10
@uranusjr
Copy link
Member

Do we not need to perform the cheks if the file exists?

@chrahunt
Copy link
Member Author

Based on the existing logic, the file cannot exist. This is described more in this commit and the assertion was added here to ensure that remains the case or fails.

These statements are equivalent, so we exchange them. This will make it
easier to factor the download_dir concerns out of these functions.
@chrahunt chrahunt force-pushed the refactor/operations-prepare branch from 6e12a9d to 7dea92e Compare December 13, 2019 05:47
@chrahunt
Copy link
Member Author

Those changes weren't really critical, so I've dropped the commits.

@uranusjr
Copy link
Member

uranusjr commented Dec 13, 2019

Maybe we can add a comment pointing here so future maintainers wanting to delete the block don’t need to go through the same discussion again. Or if someone wants to reuse that s/o/a prompt they can maybe move it to somewhere else instead. Otherwise 👍

@pradyunsg pradyunsg merged commit b9bdad2 into pypa:master Dec 13, 2019
@chrahunt chrahunt deleted the refactor/operations-prepare branch December 14, 2019 01:39
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jan 13, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants