-
-
Notifications
You must be signed in to change notification settings - Fork 27
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: Adds logic to fetch specific keys for a product #244
feat: Adds logic to fetch specific keys for a product #244
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #244 +/- ##
==========================================
- Coverage 95.06% 94.76% -0.30%
==========================================
Files 5 5
Lines 324 382 +58
==========================================
+ Hits 308 362 +54
- Misses 16 20 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Great job, I just added a suggestion for readability.
folksonomy/api.py
Outdated
query = """ | ||
SELECT json_agg(j)::json FROM ( | ||
SELECT * FROM folksonomy WHERE product = %s AND owner = %s | ||
""" | ||
params = [product, owner] | ||
|
||
if keys_list: | ||
placeholders = ', '.join(['%s'] * len(keys_list)) | ||
query += f" AND k IN ({placeholders})" | ||
params.extend(keys_list) | ||
|
||
query += " ORDER BY k) as j;" |
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.
Please rewrite it to use f-string to improve readability
query = """ | |
SELECT json_agg(j)::json FROM ( | |
SELECT * FROM folksonomy WHERE product = %s AND owner = %s | |
""" | |
params = [product, owner] | |
if keys_list: | |
placeholders = ', '.join(['%s'] * len(keys_list)) | |
query += f" AND k IN ({placeholders})" | |
params.extend(keys_list) | |
query += " ORDER BY k) as j;" | |
params = [product, owner] | |
if keys_list: | |
placeholders = ', '.join(['%s'] * len(keys_list)) | |
key_clause = f"k IN ({placeholders})" | |
params.extend(keys_list) | |
else: | |
key_clause = f"TRUE" | |
query = f""" | |
SELECT json_agg(j)::json FROM ( | |
SELECT * FROM folksonomy | |
WHERE product = %s AND owner = %s AND {key_clause} | |
ORDER BY k | |
) as j;" |
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.
made the required changes, ready to merge! thanks @alexgarel
folksonomy/api.py
Outdated
response: Response, | ||
product: str, | ||
owner: str = '', | ||
keys: str = Query(None, description="Comma-separated list of keys to filter by"), |
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.
Maybe we can specify that without keys parameter, it returns all keys.
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.
made the change, added more clear description
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.
I approve your PR @kirtanchandak still you have a conflict to resolve. I also added a comment.
@alexgarel ready to merge resolved merge conflicts and made the description change |
I have tested it, works fine. I'm going to merge. Thanks a lot! |
What
Adds logic to fetch specific keys related to a product in a comma-separated manner
http://127.0.0.1:8000/product/3760256070970?keys=taste
http://127.0.0.1:8000/product/3760256070970?keys=taste,size
Screenshot
Fixes bug(s)
#236