Skip to content
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

Don't call download completion handler twice #58

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jamitJona
Copy link
Contributor

Description

Evaluates the error in Client.downloadExecutor(_ downloadTask: URLSessionDownloadTask, didCompleteWithError error: Error?) to avoid the completion handler call in case the error is nil.

Solves #57

@jamitJona jamitJona linked an issue Dec 2, 2020 that may be closed by this pull request
@jamitJona jamitJona changed the title Don't call download competion handler twice Don't call download completion handler twice Dec 2, 2020
@@ -356,7 +356,10 @@ extension Client: DownloadExecutorDelegate {

public func downloadExecutor(_ downloadTask: URLSessionDownloadTask, didCompleteWithError error: Error?) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should rename the delegate function to didFailWithError error: Error? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. This is basically the same naming as in the URLSessionDownloadDelegate. We just "convert" two delegate methods (didCompleteWithError and didFinishDownloadingTo) into one completion. And therefore we should keep track when it's appropriate to call the completion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm ok, then we could leave it as is. Can you please add documentation or rethink the visibility of the function? Is it necessary, that it is available from outside of the framework?

@JensK611
Copy link
Contributor

@jamitJona What's the actual state of this PR?

@JensK611
Copy link
Contributor

JensK611 commented Jul 6, 2021

@jamitJona maybe you can give us an update about this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DownloadHandler.CompletionHandler called twice
2 participants