Skip to content

Feature/lsr reaper - #8

Open
sray014 wants to merge 13 commits into
mainfrom
feature/lsr-reaper
Open

Feature/lsr reaper#8
sray014 wants to merge 13 commits into
mainfrom
feature/lsr-reaper

Conversation

@sray014

@sray014 sray014 commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Add NWS Local Storm Report (LSR) reaper

@sray014
sray014 requested a review from cheginit August 6, 2026 15:48

@cheginit cheginit left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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:120 annotates _fetch as a bare dict where the rest of the module is fully typed.
  • lsr.py:115 builds the URL by f-string where asos.py:133 uses urlencode.
  • lsr.py:42 uses >= where asos.py:40 and usace.py:67 use >, so equal timestamps behave differently across reapers. Worth resolving deliberately.
  • test_lsr_reapers.py:12 has an extra blank line that pixi r lint auto-fixes (I001).
  • PLR0917 fires on __init__ but also on asos.py, nwp.py and usace.py, so it is pre-existing rather than this PR's problem.

Comment thread src/cosecha/reaping/lsr.py Outdated
Comment thread src/cosecha/reaping/lsr.py Outdated
Comment thread src/cosecha/reaping/lsr.py Outdated
Comment thread src/cosecha/reaping/lsr.py Outdated
Comment thread src/cosecha/reaping/lsr.py Outdated
Comment thread tests/test_reaping/test_lsr_reapers.py Outdated
@sray014
sray014 requested a review from cheginit August 14, 2026 17:47

@cheginit cheginit left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread src/cosecha/reaping/base.py Outdated
Comment thread src/cosecha/reaping/lsr.py Outdated
@sray014
sray014 requested a review from cheginit August 17, 2026 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants