-
Notifications
You must be signed in to change notification settings - Fork 15
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
Image thumbnail retrieval implemented #54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some typos. Other than that, it looks fantastic!
Approving this PR. For the typos, it's not really a big deal. You can make a commit and self merge the branch if you like, or just leave it, I can solve it in my coming PR as well.
Great job. :D
Awesome! Merging. |
def __str__(self) -> str: | ||
return f"""An exception occured, "{self._resolution}" is not a supported image size | ||
|
||
Hint: Supported image sizes are: 256, 1024, and 2048 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OmarMuhammedAli will this be exposed to the user? Would be helpful to provide the hint in case they otherwise do not know, so if they enter 320 it can mention the hint in the error message maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbeddow Yes, this hint is exposed to the user. The controller first checks for the given resolution, and if it's not one of the supported ones, it doesn't go on with requesting the thumbnail from the API, and it raises an exception that shows the error message alongside the hint.
|
||
:param size: Option for the thumbnail size, ranging from 320 to 2048 width | ||
:type size: int | ||
:param resolution: Option for the thumbnail size, with available resolutions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should default to 1024 if it is unspecified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's already implemented. I'll update the docstring to reflect that as well!
Related Issue(s) #17
Proposed Changes
image_thumbnail
driver function.get_imgae_thumbnail
controller to hold business logicInvalidImageResolution
to handle unsupported input resolution exceptionsInvalidImageKey
to handle invalid input image ID/key exceptions.Example Usage