Skip to content

Conversation

@YomesInc
Copy link
Contributor

@YomesInc YomesInc commented Jun 5, 2021

Brief Summary of Changes

User can now pass ‘urls’ as a source of sprite/multi.

What Does This PR Address?

  • GitHub issue (Add reference - #XX)
  • Refactoring
  • New feature
  • Bug fix
  • Adds more tests

Are Tests Included?

  • Yes
  • No

Reviewer, Please Note:

@YomesInc YomesInc requested a review from patrick-tolosa June 5, 2021 04:53
Copy link
Contributor

@patrick-tolosa patrick-tolosa left a comment

Choose a reason for hiding this comment

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

Hey,
Please add a detailed description in the PR of what was changed, and why.

This PR is very complex to be reviewed without any context.
I can see that a refactor was done around the api_download_url but I'm not sure I understand why.

@YomesInc
Copy link
Contributor Author

Since implemented feature assumes similar changes to multiple methods, we've added build_multi_and_sprite_params similar to other SDKs, e.g. PHP.

Another part of the PR is adding download_generated_sprite and download_multi. By looking at it, all download methods have similar logic: set mode=download parameter, sign parameters, build a URL. And this logic applies not only to new download_ methods, but to download_archive_url as well. Instead of having 3 places doing same steps, we've decided to implement api_download_url.

As for tests, we've implemented a couple of helpers:

  • beASprite
  • beASignedDownloadUrl
  • beAMulti
    This way we get a deeper check than usually done in unit tests and tests are more explicit about expectations. E.g. by expect(result).to.beASprite() we can see that we expect a sprite and an extensive set of checks makes sure that we have a sprite rather than a somewhat similar object.

That's the reasoning for the changes made. Please let us know if you'd like more specific details for any part of the changes.

@patrick-tolosa
Copy link
Contributor

Thanks @YomesInc
Lets fix the conflicts and then we can review this PR.

@patrick-tolosa patrick-tolosa merged commit 7e6dbe7 into master Sep 12, 2021
@patrick-tolosa patrick-tolosa deleted the feature/allow-multi-and-sprite-with-urls branch September 12, 2021 12:29
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.

3 participants