Add a targetFile option for downloading straight to a file - #62
Draft
fingolfin wants to merge 1 commit into
Draft
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 190 +38
Branches 15 22 +7
========================================
+ Hits 145 179 +34
- 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
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 largestfile it can fetch is bounded by memory.
With
opt.targetset to a filename the body goes straight to that file viaCURLOPT_WRITEDATA, and the result record has noresult. A 200 MB downloadcosts 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 older
utils. With a nameutilsdoes not forward, all fourversion combinations work, and gap-packages/utils# makes
utilspasstargeton 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 needsthe partial file. I chose the safe default; resume would want an opt-in.
Written with Claude Opus 5 via Claude Code; reviewed by me.