Add a targetFile option for downloading straight to a file - #62
Draft
fingolfin wants to merge 2 commits into
Draft
Add a targetFile option for downloading straight to a file#62fingolfin wants to merge 2 commits into
targetFile option for downloading straight to a file#62fingolfin wants to merge 2 commits into
Conversation
'DownloadURL' and friends built the whole response body as a GAP string and returned it, so the largest file one could fetch was bounded by memory. That matters for the use case this package is increasingly put to: fetching data sets on behalf of other packages. With opt.target set to a filename, the body is written straight to that file via CURLOPT_WRITEDATA as it arrives, and the result record has no 'result' component. Downloading 200 MB now costs 158 MB peak RSS instead of 334 MB. If the request fails the file is removed, so a caller may test whether it exists to decide whether it got the data. Note that CURL_REQUEST now takes 9 arguments rather than 8. It is not documented and CurlRequest fills in the new one, but anyone calling the kernel function directly has to adjust. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #62 +/- ##
========================================
- Coverage 95.3% 94.2% -1.2%
========================================
Files 3 3
Lines 152 191 +39
Branches 15 22 +7
========================================
+ Hits 145 180 +35
- Misses 3 5 +2
- Partials 4 6 +2
🚀 New features to boost your workflow:
|
fingolfin
marked this pull request as draft
August 16, 2026 07:24
The utils package uses 'target' as its own Download option and passes the whole option record on to DownloadURL, so honouring 'target' here would break `Download(url, rec(target := ...))` with an older utils. A name utils does not forward keeps every combination of versions working. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
target option for downloading straight to a filetargetFile option for downloading straight to a file
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
DownloadURLbuilds the whole response body as a GAP string, so the largest file it can fetch is bounded by memory.With
opt.targetFileset to a filename the body goes straight to that file viaCURLOPT_WRITEDATA, and the result record has noresult. A 200 MB download costs 158 MB peak RSS instead of 334 MB; the written file's SHA256 matches. On failure the file is removed.The option is called
targetFile, nottarget, on purpose:utilsusestargetfor its ownDownloadoption and forwards the whole record toDownloadURL, so taking that name would breakDownload(url, rec(target := ...))for anyone on an olderutils. Verified: with the option namedtarget, a releasedutilsagainst this branch raisesRecord Element: '<rec>.result' must have an assigned value; withtargetFileall four old/new combinations work. gap-packages/utils#104 makesutilspasstargeton astargetFileto get the memory saving.Tested against GAP 4.12.2, the declared minimum, and current GAP.
Two open questions:
CURL_REQUESTgoes from 8 arguments to 9. It is undocumented andCurlRequestfills in the new one, but direct callers break —tst/errors.tstasserted the arity. Keep it at 8 instead?CURLOPT_RESUME_FROM, which needs the partial file. I chose the safe default; resume would want an opt-in.Written with Claude Opus 5 via Claude Code.