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

Optional completion handler #407

Merged
merged 1 commit into from
Mar 7, 2020
Merged

Optional completion handler #407

merged 1 commit into from
Mar 7, 2020

Conversation

robertmryan
Copy link
Contributor

Goals ⚽

The goals are:

  • More concise API:

    When downloading images (e.g. as part of a prefetch process), you don’t need a completion handler closure, but right now we have to supply one although the parameter is optional. For example, if you don’t have a completion handler, you have to do:

    imageDownloader.download(request, completion: nil)
    

    With this change, we can omit that completion parameter entirely:

    imageDownloader.download(request)
    
  • More consistent API:

    Everywhere else in the API, the optional completion handlers have a default value of nil. But the download method of ImageDownloader does not.

Implementation Details 🚧

Just supplied a default parameter of nil for the completion handler for this one method.

Testing Details 🔍

@robertmryan
Copy link
Contributor Author

FWIW, I don’t know why that iOS test target is failing as part of the CI, as it’s not failing when I run tests locally. Perhaps because the two requests are downloading the exact same image?

@jshier
Copy link
Contributor

jshier commented Mar 2, 2020

Those tests are racy in a way that only shows up in CI. We’re still trying to figure it out. In the meantime, it’s fine for PRs.

Copy link
Contributor

@jshier jshier left a comment

Choose a reason for hiding this comment

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

👍 The documentation for the method already says it defaults to nil, so this just seems like a bug.

@jshier jshier merged commit b1aeb54 into Alamofire:master Mar 7, 2020
@jshier jshier added this to the 4.0.3 milestone Mar 7, 2020
@robertmryan robertmryan deleted the optional-completion-handler branch March 7, 2020 22:25
ryndinol pushed a commit to ryndinol/AlamofireImage that referenced this pull request Apr 26, 2021
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.

2 participants