#curl_download: default try_partial to false
#13179
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review period will end on 2022-04-25 at 16:12:57 UTC. |
BrewTestBot
added
the
waiting for feedback
MikeMcQuaid
approved these changes
Apr 22, 2022
When its `try_partial` argument is `true`, `#curl_download` makes a `HEAD` request before downloading the file using `#curl`. Currently `try_partial` defaults to `true`, so any `#curl_download` call that doesn't explicitly specify `try_partial: false` will make a `HEAD` request first. This can potentially involve several requests if the URL redirects, so it can be a bit of unnecessary overhead when a partial download isn't needed. Partial downloads are generally only useful when we're working with larger files, however there's currently only one place in brew where `#curl_download` is used and this is the case: `CurlDownloadStrategy`. The other `#curl_download` calls are fetching smaller [text] files and don't need to support partial downloads. This commit changes the default `try_partial` value to `false`, making partial downloads opt-in rather than opt-out. We want `try_partial` to continue to default to `true` in `CurlDownloadStrategy` and there are various ways to accomplish this. In this commit, I've chosen to update its `#initialize` method to accept a `try_partial` argument that defaults to `true`, as this value can also be used in classes that inherit from `CurlDownloadStrategy` (e.g., `HomebrewCurlDownloadStrategy`). This instance variable is passed to `#curl_download` in related methods, effectively maintaining the previous `try_partial: true` value, while also allowing this value to be overridden when necessary. Other uses of `#curl_download` in brew are `Formulary::FromUrlLoader#load_file` and `Cask::CaskLoader::FromURILoader#load`, which did not provide a `try_partial` argument but should have been using `try_partial: false`. With the `try_partial: false` default in this commit, these calls are now fine without a `try_partial` argument. The only other use of `#curl_download` in brew is `SPDX#download_latest_license_data!`. These calls were previously using `try_partial: false` but we can now omit this argument with the new `false` default (aligning with the above).
samford
force-pushed
the
curl-download-try_partial-false
branch
from
6190b1d
to
3f7d9f8
Compare
MikeMcQuaid
approved these changes
Apr 22, 2022
|
Review period ended. |
BrewTestBot
removed
the
waiting for feedback
BrewTestBot
approved these changes
Apr 25, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?When its
try_partialargument istrue,#curl_downloadmakes aHEADrequest before downloading the file using#curl. Currentlytry_partialdefaults totrue, so any#curl_downloadcall that doesn't explicitly specifytry_partial: falsewill make aHEADrequest first. This can potentially involve several requests if the URL redirects, so it can be a bit of unnecessary overhead when a partial download isn't needed.Partial downloads are generally only useful when we're working with larger files, however there's currently only one place in brew where
#curl_downloadis used and this is the case:CurlDownloadStrategy. The other#curl_downloadcalls are fetching smaller [text] files and don't need to support partial downloads.This PR changes the default
try_partialvalue tofalse, making partial downloads opt-in rather than opt-out.We want
try_partialto continue to default totrueinCurlDownloadStrategyand there are various ways to accomplish this. In this PR, I've chosen to update its#initializemethod to accept atry_partialargument that defaults totrue, as this value can also be used in classes that inherit fromCurlDownloadStrategy(e.g.,HomebrewCurlDownloadStrategy). This instance variable is passed to#curl_downloadin related methods, effectively maintaining the previoustry_partial: truevalue, while also allowing this value to be overridden when necessary.Other uses of
#curl_downloadin brew areFormulary::FromUrlLoader#load_fileandCask::CaskLoader::FromURILoader#load, which did not provide atry_partialargument but should have been usingtry_partial: false. With thetry_partial: falsedefault in this PR, these calls are now fine without atry_partialargument.The only other use of
#curl_downloadin brew isSPDX#download_latest_license_data!. These calls were previously usingtry_partial: falsebut we can now omit this argument with the newfalsedefault (aligning with the above).