-
Notifications
You must be signed in to change notification settings - Fork 125
FileDownloadDelegate: pass response header through #680
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
Conversation
Currently `FileDownloadDelegate` doesn't pass the response header through in its `Response` associated type, which is satisfied by the `Progress` inner type declaration. This makes it hard to use in the Swift Concurrency context, since it's no trivial to pass response header around from `reportHead` closure. Let's fix that by adding a new field that passes the response header in the delegate's response. This change is additive and is not source-breaking.
Closing this in favour of #681 |
I feel like maybe this shouldn't have been closed. It's not super ergonomic to have to capture the response head via the var head: HTTPResponseHead?
let delegate = try FileDownloadDelegate(path: "/some/tmp/file.tmp",
pool: nil,
reportHead: {
head = $1
if $1.status.code != 200 {
$0.cancel()
}
},
reportProgress: nil
)
let result = try await HTTPClient.shared.execute(request: request, delegate: delegate).get()
if let head {
// do something with head
} |
The fix in 681 was suitable for the use-case in question, where the |
In my case I want to inspect the "content-disposition" field to determine a suitable user-facing file name. |
Ok, so I think it's worth us adding this flow. @dnadoba any objections on your end? |
I'm happy to make a PR with the change. I don't think the |
We cannot replace the I'm fine if we add it. However, if we want to add it we should make the head non-optional and require it as opposed to what is proposed in this PR. This can be done safely because we only report progress (or finish the request and return |
Currently
FileDownloadDelegate
doesn't pass the response header through in itsResponse
associated type, which is satisfied by theProgress
inner type declaration. This makes it hard to use in the Swift Concurrency context, since it's no trivial to pass response header around fromreportHead
closure. Let's fix that by adding a new field that passes the response header in the delegate's response.This change is not source-breaking and is purely additive, since no existing properties were changed.