Take into consideration the time component in endtime
#97
Conversation
pydantic 2 changed the text of the exception it raises, that's why we need to update the tests, too.
Codecov Report
@@ Coverage Diff @@
## master #97 +/- ##
==========================================
+ Coverage 89.98% 90.01% +0.03%
==========================================
Files 13 13
Lines 1008 1012 +4
Branches 114 150 +36
==========================================
+ Hits 907 911 +4
+ Misses 57 55 -2
- Partials 44 46 +2
|
| @@ -48,7 +48,7 @@ def get_erddap_url( | |||
| response: str = "csvp", | |||
| ) -> str: | |||
| erddap = erddapy.ERDDAP( | |||
| server=dataset.server_url, | |||
| server=str(dataset.server_url), | |||
There was a problem hiding this comment.
I haven't used ERDDAP module, so I'm not sure if this is relevant. But is it possible for the value to become None and making it str() can break some check downstream?
There was a problem hiding this comment.
Well the ERDDAP stuff hasn't been used/tested in some time. If we decide to add a Provider using an ERDDAP server we will need to revisit it anyhow. For now I just made the change so that the tests pass.
This specific change is actually caused by pydantic 2+ which changed some APIs.
This involves several changes:
1. We rename `resolve_date` to `resolve_timestamp` since we no longer only deal with dates
2. The signature and the type of returned objects in`resolve_timestamp()` has changed.
More specifically the function became quite a bit more capable and complex.
It gained extra arguments that allow it to return both timezone-aware and
naive Timestamps.
3. We change the Literal value `TODAY` to `now`
Fixes oceanmodeling#96
| @@ -279,7 +279,7 @@ def get_usgs_station_data( | |||
| while rate_limit.reached(identifier="USGS"): | |||
| wait() | |||
|
|
|||
| endtime = resolve_date(endtime) | |||
| endtime = resolve_timestamp(endtime, timezone_aware=False).date() | |||
There was a problem hiding this comment.
@SorooshMani-NOAA AFAI can tell dataretrieval/USGS handle the time component just fine. For example:
import dataretrieval.nwis as nwis
print(len(nwis.get_record(sites='03339000', service='iv', start='2017-12-31', end='2018-01-01'))) # prints 192
print(len(nwis.get_record(sites='03339000', service='iv', start='2017-12-31T12:00:00', end='2018-01-01'))) # prints 144In that sense we don't need to call .date() in line 282. I only added that in order to keep the old behavior and to let the tests pass. Your call if we keep it as is or we remove .date().
There was a problem hiding this comment.
@pmav99 let's keep the behavior the same as before by adding .date() for now. Thanks for notifying me.
This involves several changes:
resolve_datetoresolve_timestampsince we no longer only deal with datesresolve_timestamp()has changed. More specifically the function became quite a bit more capable and complex. It gained extra arguments that allow it to return both timezone-aware and naive Timestamps.TODAYtonowFixes #96