-
Notifications
You must be signed in to change notification settings - Fork 214
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
Add checks for required parameters to Provider scripts (K-R) #1884
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.
Nice! I'm surprised there were not more tests which were affected. Do you think it makes sense to go through and add test cases for the providers which check against a few falsy values?
9f83a94
to
cc606ca
Compare
df2a4b0
to
4b17bfe
Compare
is None
checks with not
checksc4c5958
to
d80d706
Compare
3833b44
to
d611eba
Compare
d611eba
to
f2697cb
Compare
9ce9453
to
6a749c9
Compare
f2697cb
to
629b1a8
Compare
6a749c9
to
127781b
Compare
629b1a8
to
a882797
Compare
a882797
to
0c599b4
Compare
efeff99
to
73041ec
Compare
@AetherUnbound, I re-requested your review because this PR has changed a lot since your approval. |
catalog/tests/dags/providers/provider_api_scripts/test_metropolitan_museum.py
Show resolved
Hide resolved
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.
LGTM. Thank you for caring about splitting the changes for easier reviews and adding type hints ✨
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.
LGTM, no blocking requests. Thanks for all the additional cleanup and type hints added as well :)
media_data = media[size] | ||
break | ||
image_url = media_data.get("uri") | ||
if image_url is not None: | ||
if image_url := media_data.get("uri"): |
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.
Could image_url
still be falsy here?
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 think it shouldn't because we check that media[size].get("uri")
is not falsy above (I updated the check).
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.
Unless we didn't find any size in media
, in which case media_data
is still {}
. So we still have to have the check.
title_info = mods.get("titleInfo") | ||
if isinstance(title_info, list) and title_info: | ||
title_info = title_info[0] | ||
return "" if title_info is None else title_info.get("title", {}).get("$") |
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.
Should this is None
check also be changed to a check for falsy values?
Fixes
Fixes #1883 by @obulat
Description
This PR replaces
is None
checks for required values withif <value>
checks because falsy values such as empty string or empty list should also short-circuit the provider API scripts.Testing Instructions
All the CI tests should pass.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin