Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Directly use python_cmr's temporal methods. #546

Merged
merged 1 commit into from
Apr 30, 2024
Merged

Directly use python_cmr's temporal methods. #546

merged 1 commit into from
Apr 30, 2024

Conversation

chuckwondo
Copy link
Collaborator

@chuckwondo chuckwondo commented Apr 30, 2024 •
edited by github-actions bot
Loading

Bump python_cmr version to make use of additional logic in the temporal methods that were pushed from earthaccess up to python_cmr itself.

Fixes #190 #330

📚 Documentation preview 📚: https://earthaccess--546.org.readthedocs.build/en/546/

@itcarroll @chuckwondo
Directly use python_cmr's temporal methods.
3f43a01 
Bump `python_cmr` version to make use of additional logic in
the `temporal` methods that were pushed from `earthaccess`
up to `python_cmr` itself.

Fixes nsidc#190 nsidc#330
@chuckwondo chuckwondo mentioned this pull request Apr 30, 2024
Closed
@chuckwondo
Copy link
Collaborator Author

@betolink or @mfisher87, would one of you be able to give this a quick review?

This was originally from @itcarroll, who handed things off to me to fix the build issues he was encountering.

The gist of it is an upgrade to python_cmr 0.10.0 because Ian had pushed his changes to the temporal methods upstream, which then allowed him to remove the logic from the temporal methods in earthaccess (simply calling the superclass). As discussed elsewhere, we did not remove the temporal methods completely from earthaccess, as we still don't have a solution for ensuring a complete set of docs in such a case. (I believe the solution might be that we need to get python_cmr to publish docs, which should create an index that we could then pull into our own without the need to duplicate docstrings.)

mfisher87
mfisher87 approved these changes Apr 30, 2024
View reviewed changes
Copy link
Collaborator

@mfisher87 mfisher87 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great work @itcarroll and @chuckwondo. Awesome to delete code 😁

stubs/cmr/queries.pyi
date_from: Optional[Union[str, datetime]],
date_to: Optional[Union[str, datetime]],
date_from: Optional[Union[str, date, datetime]],
date_to: Optional[Union[str, date, datetime]],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we get to delete our stubs when python-cmr v0.11.0 is released?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, which reminds me, I forgot to ask when that will happen.

@chuckwondo chuckwondo merged commit 2c42a0f into nsidc:main Apr 30, 2024
12 checks passed
@chuckwondo chuckwondo deleted the temporal branch April 30, 2024 09:12
@chuckwondo chuckwondo mentioned this pull request Apr 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfisher87 mfisher87 mfisher87 approved these changes

@betolink betolink Awaiting requested review from betolink

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

H:M:S for Date_To in the Temporal function defaults to 0:0:0
3 participants
@chuckwondo @mfisher87 @itcarroll