Feature/lsr reaper - #8
Conversation
cheginit
left a comment
There was a problem hiding this comment.
Reviewed against the live IEM endpoint. Details are inline; this note covers only what has no line in this diff to attach to.
The parse_date_range helper suggested on _validate_params fixes LSR, but the same three date bugs are already in asos.py, usace.py and nwis.py, which each repeat the same five-line parse block:
ASOS mixed tz -> TypeError Cannot compare tz-naive and tz-aware timestamps
USACE mixed tz -> TypeError Cannot compare tz-naive and tz-aware timestamps
NWIS mixed tz -> TypeError Cannot compare tz-naive and tz-aware timestamps
USACE stamp: 2024-05-21T13:00:00Z <- literal Z pasted onto -05:00 wall time
(usace.py:144, true UTC is 18:00:00)
Per reaper that is: swap the try/except for a parse_date_range call, drop the date comparison from _validate_params, add the import. usace.py:144 also needs the UTC conversion before strftime, since it formats with a hardcoded Z; nwis.py:119 uses isoformat() and is already fine on that axis. Expect test_asos_reapers.py:27 and test_nwis_reapers.py:26 to need tz="UTC" added to their assertions, since naive input becomes tz-aware. That is the behavior change working as intended rather than a regression, but it does mean the change is not purely mechanical. Doing all four together avoids leaving three reapers carrying a bug this PR already documents, and it keeps the helper pinned by more than one caller. A follow-up issue works too, as long as the helper itself lands here.
Smaller things, none blocking:
lsr.py:120annotates_fetchas a baredictwhere the rest of the module is fully typed.lsr.py:115builds the URL by f-string whereasos.py:133usesurlencode.lsr.py:42uses>=whereasos.py:40andusace.py:67use>, so equal timestamps behave differently across reapers. Worth resolving deliberately.test_lsr_reapers.py:12has an extra blank line thatpixi r lintauto-fixes (I001).PLR0917fires on__init__but also onasos.py,nwp.pyandusace.py, so it is pre-existing rather than this PR's problem.
cheginit
left a comment
There was a problem hiding this comment.
Re-checked the six earlier findings against this head: all six are resolved and verified against the live IEM endpoint. Two new things came in with the changes, noted inline.
Add NWS Local Storm Report (LSR) reaper