-
Notifications
You must be signed in to change notification settings - Fork 84
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
improve ISO 8601 parsing for temporal filter #486
Conversation
Co-authored-by: Joseph H Kennedy <me@jhkennedy.org>
Believe the pre 3.10 checks are failing due to |
Yes, using from typing import Union
def foo(bar: Union[int, float]): For from typing import Optional
def fizz(buzz: Optional[thing]): # buzz can be None or thing But that's an easy circle back once we're happy with the implementation |
Test error is |
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.
What I don't love about the simplification here is the call to parse when it may not have been necessary (if we already had a datetime).
I think that roundtrip significantly simplifies the logic in the function; your latest version has nested branches/leafs which I find makes it harder to follow the logic all the way through without spending some time in the function.
That said, we've got really good tests now, so I'm fine with whatever implementation you prefer that passes the tests.
With nasa/python_cmr#31 merged, I believe we can close this as a won't fix in favor of a new PR that passes user's temporal input directly to python_cmr. Sound good? |
Fixes #330.
As suggested by @jhkennedy, earthaccess will now pass a
datetime
object (orNone
) tocmr
, essentially replacing the basic string parsing thatcmr
does (usingdatetime.strptime
) with flexible and timezone aware parsing bydateutil.parser
.The change created the opportunity to create a new suite of tests for possible user inputs, that includes checking exceptions.
📚 Documentation preview 📚: https://earthaccess--486.org.readthedocs.build/en/486/