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

Tests for throwing URLRequestConvertible #392

Closed

Conversation

hybridcattt
Copy link
Contributor

@hybridcattt hybridcattt commented Feb 12, 2020

Issue Link 🔗

Unit tests for #241

Goals ⚽

Ensure the library correctly supports throwing URLRequestConvertible requests

Implementation Details 🚧

I've started fixing #241 but then noticed that a fix is already present on master. I already added the tests at that point, so hope these are useful still.

  • Added corresponding tests for ImageDownloader, UIButton and UIImageView extensions
  • Minor code refactoring in ImageDownloader to make it easier to understand and make it safer - preventing variable shadowing that can confuse the reader.

Testing Details 🔍

The PR mainly contains new tests for pre-existing functionality, and the minor refactoring in ImageDownloader is covered by existing tests which pass.
There are couple of failing tests which seem to be failing on master too.

@hybridcattt hybridcattt requested a review from cnoon February 12, 2020 21:16
@cnoon
Copy link
Member

cnoon commented Feb 23, 2020

Thank you for putting this together! 🍻

I just pushed both sets of changes into master as ba64b1b and 529ea5b while maintaining your attribution. I made a few minor tweaks to the project organization and such. These changes will go out shortly in AFI 4.0.0.

Thanks again!

@cnoon cnoon closed this Feb 23, 2020
@cnoon cnoon self-assigned this Feb 23, 2020
@cnoon cnoon added this to the 4.0.0 milestone Feb 23, 2020
ryndinol pushed a commit to ryndinol/AlamofireImage that referenced this pull request Apr 26, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants