-
Notifications
You must be signed in to change notification settings - Fork 107
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
Type annotate the temporal module #604
Conversation
👈 Launch a binder notebook on this branch for commit 7f92261 I will automatically update this comment whenever this PR is modified 👈 Launch a binder notebook on this branch for commit e805f01 👈 Launch a binder notebook on this branch for commit 7b57037 👈 Launch a binder notebook on this branch for commit 224ca75 👈 Launch a binder notebook on this branch for commit df965c2 👈 Launch a binder notebook on this branch for commit 7751e26 👈 Launch a binder notebook on this branch for commit d037e11 👈 Launch a binder notebook on this branch for commit 094d3f0 👈 Launch a binder notebook on this branch for commit 97a5544 👈 Launch a binder notebook on this branch for commit b0eb2b9 👈 Launch a binder notebook on this branch for commit af907f3 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## development #604 +/- ##
===============================================
- Coverage 71.78% 66.79% -4.99%
===============================================
Files 38 36 -2
Lines 3133 3078 -55
Branches 599 593 -6
===============================================
- Hits 2249 2056 -193
- Misses 773 933 +160
+ Partials 111 89 -22 ☔ View full report in Codecov by Sentry. |
7f92261
to
e805f01
Compare
e805f01
to
7b57037
Compare
ca3f553
to
3da94ef
Compare
icepyx/core/temporal.py
Outdated
@@ -261,14 +271,18 @@ def validate_date_range_date(date_range, start_time=None, end_time=None): | |||
return _start, _end | |||
|
|||
|
|||
def validate_date_range_dict(date_range, start_time=None, end_time=None): | |||
def validate_date_range_dict( | |||
date_range: dict[str, dt.datetime], |
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.
date_range: dict[str, dt.datetime], | |
date_range: dict[str, dt.datetime, dt.date], |
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.
Good catch! I see what you're going for, the correct way to write that would be dict[str, Union[dt.datetime, dt.date]]
, which means "a dict with str
keys, and values which can be either dt.datetime
or dt.date
."
Dates and datetimes are related, so I think we can equivalently annotate dict[str, dt.date]
:
>>> import datetime as dt
>>> datetime = dt.datetime.now()
>>> date = dt.datetime.now().date()
>>> datetime
datetime.datetime(2024, 9, 25, 12, 47, 38, 861853)
>>> date
datetime.date(2024, 9, 25)
>>> isinstance(datetime, dt.date)
True
>>> isinstance(datetime, dt.datetime)
True
>>> isinstance(date, dt.date)
True
>>> isinstance(date, dt.datetime)
False
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.
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.
Based on your description, it should be:
dict[str, Union[str, dt.date]]
since str is also a valid input for the values.
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 see, thank you!
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.
97a5544 - I should have read the doctests more closely 🔔
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.
Co-authored-by: Jessica Scheick <[email protected]>
for more information, see https://pre-commit.ci
Co-Authored-By: Jessica Scheick <[email protected]>
Co-authored-by: Jessica Scheick <[email protected]>
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.
thanks for adding types to this module, @mfisher87!
Thanks for the thorough review 🙇 I don't understand why the unit tests aren't running here. Re-opening PR to trigger. |
|
Important
To be merged only after merging #598 and then rebasing
Annotating this module was pretty quick! No typeguards or ignores needed.