-
Notifications
You must be signed in to change notification settings - Fork 86
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
Allow single file URL inputs for earthaccess.download
#347
Conversation
earthaccess/api.py
Outdated
if isinstance(granules, DataGranule): | ||
granules = [granules] | ||
if isinstance(granules, (DataGranule, str)): | ||
granules = cast(Union[List[DataGranule], List[str]], [granules]) |
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 cast
is a little messy, but is to make mypy
happy. I'm not a typing expert -- definitely open to other suggestions that make this less involved.
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 one feels like it should be easy 😆 It's such a common pattern for a function to accept one or many of a thing. I'm sure there's a term-of-art of this, but my memory and google skills are failing me.
This method avoids telling the typechecker "trust us" so I think it's a bit better, but it's also still pretty messy. With the combined isinstance
check, Mypy sees thing
as str | Thing
, but then after list()
ing it Mypy infers list[str | Thing]
instead of what I expected: list[str] | list[thing]
. That's why there are two separate isinstance
checks :\ .
class Thing:
pass
def foo(things: Thing | str | list[Thing] | list[str]) -> list[str] | list[Thing]:
thing_list: list[str] | list[Thing]
if isinstance(things, Thing):
thing_list = [things]
elif isinstance(things, str):
thing_list = [things]
return thing_list
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'm going to share this out and see if anyone wants to play code golf. I'm working on a TypeScript project right now and thinking about the Python type system is hurting me 🤣
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.
An entry using a generic function and switch statement: https://gist.github.com/kbeamnsidc/4ffa3708677fa5b36623071443a194c7
Co-authored-by: Matt Fisher <matt.fisher@nsidc.org>
I'll plan to merge this PR in a few hours if there are no further comments. It should be a relatively non-controversial change and I'm happy to own any follow-up work, if there is any. |
I'd like to avoid the |
I don't really care either way. Just updated to avoid the cast |
Similar to #317 but for a single
https://...
URL instead of a single data granule