Skip to content

Add head property to FileDownloadDelegate's Progress/Response struct #811

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

Merged
merged 10 commits into from
Feb 18, 2025

Conversation

gregcotten
Copy link
Contributor

@gregcotten gregcotten commented Feb 13, 2025

I needed a way to use a FileDownloadDelegate task to fish out the recommended file name.

let response = try await downloadTask.get()

// access content-disposition
response.head.headers.first(name: "Content-Disposition")

The head property is an explicitly unwrapped optional because there is no "default value" to set it to, and it won't be accessed by the user until it's already been set anyway. This is a little inelegant, so I could change it to something like below where I fill in bogus init data, but that seems worse for some reason.

public struct Progress: Sendable {
    public var totalBytes: Int?
    public var receivedBytes: Int
    public var head: HTTPResponseHead
}

private var progress = Progress(
    totalBytes: nil,
    receivedBytes: 0,
    head: .init(
        version: .init(major: 0, minor: 0),
        status: .badRequest
    )
)

@gregcotten
Copy link
Contributor Author

summoning @Lukasa and @dnadoba since this was discussed previously here: #680 (comment)

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 14, 2025

Hi @gregcotten, thanks for opening this and sorry for the slow time to review, it's been a busy week. I'll take a look now.

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, left a note in the diff. Can we also add at least one unit test?

it will never be seen by the library user with this data since `didReceiveHead(...)` is called before we report progress or finalize
@gregcotten
Copy link
Contributor Author

gregcotten commented Feb 14, 2025

OK @Lukasa I, at my own risk, have diverged from your suggestion as I think it's really important that the head property is never nil (since from the user's perspective it should never be). I've instead initialized it with bogus data (HTTP 0.0, status code 0) and it's replaced before the library user ever gets a hold of it.

I've also added its use to existing unit tests, but can make a unique unit test if you have any suggestions.

@gregcotten
Copy link
Contributor Author

Needless to say this should be a squash merge if accepted :)

@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Feb 17, 2025
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor nits here but this is looking good. I'll kick off CI for some more info there as well.

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 17, 2025

Looks like the formatter wants a few tweaks from you.

@gregcotten
Copy link
Contributor Author

Looks like the formatter wants a few tweaks from you.

Done!

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I really like this.

@Lukasa Lukasa enabled auto-merge (squash) February 18, 2025 15:16
@Lukasa Lukasa merged commit d05bf23 into swift-server:main Feb 18, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants