-
Notifications
You must be signed in to change notification settings - Fork 123
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
Feat 4122 compress image python #160
base: main
Are you sure you want to change the base?
Feat 4122 compress image python #160
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.
Hey 👋 awesome work on your PR! We've approved your work and it'll be merged soon!
I did also leave some minor feedback.
python/compress-image/README.md
Outdated
- **API_KEY** - Tinypng API Key or KrakenIO API Key | ||
- **SECRET_API_KEY** - KrakenIO Secret API Key |
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.
There should be 3 different variables for the different values so that a function can be deployed with all tinypng and krakenio variables and then executed with either tinypng or krakenio.
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.
Hello, we recently added a commit addressing the 3 variables for the different values. It should show now.
python/compress-image/main.py
Outdated
IMPLEMENTATIONS = { | ||
"krakenio": krakenio_impl, | ||
"tinypng": tinypng_impl, | ||
} |
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.
This approach to defining different implementations is a little odd. An object-oriented approach is more common.
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 fixed this one.
python/compress-image/main.py
Outdated
return optimized_image | ||
|
||
|
||
def tinypng_impl(variables): |
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.
methods are more commonly named as verbs.
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 fixed this one.
Addressed stnguyen90 regarding 3 different variables for TinyPNG and KrakenIO.
Feat 4122: Add compressImage function in Python
Checklist:
Closes appwrite/appwrite#4122.
Authors: Noah Jacinto, Ngoc Nguyen.
Summary
Details
README.md
contains instructions on how to run the main function.main.py
contains functions to validate requests and perform image optimization through the TinyPNG/KrakenIO provider.requirements.txt
contains Python packages and dependencies used for our build.secret.py
contains API Key Variables for Tinypng and KrakenIO. It includes API_KEY_TINYPNG, API_KEY_KRAKENIO and SECRET_API_KEY_KRAKENIO. These values should be changed to your respective api keys.test_main.py
contains unittest for themain.py
file with various scenarios like happy paths, value errors, unexpected exceptions etc.test_main.py
uses1kb.png
,1kb_result_encoded_krakenio.txt
and1kb_result_encoded_tinypng.txt
inside thetest
folder.Testing
More information about setting up the environment, refer to
README.md
.Tinypng
KrakenIO
Test Results