Infer a month that actually has the report's day (fixes #167) - #208
akrherz merged 2 commits into
Conversation
Sorry, something went wrong.
A METAR carries only the day of the month, so _handleTime guesses the rest: this month, or last month if the day is still ahead of today. When last month has no such day the guess is an impossible date and datetime raises "day is out of range for month" -- a 31st report read on March 5th lands on February 31st. Walk back to the most recent month that does have the day, bounded at twelve steps since the day comes from a \d\d group. An explicit month= is left alone: there the impossible date is the caller's assertion, not a guess, and should still raise. Fixes python-metar#167
Codecov Report❌ Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #208 +/- ##
==========================================
- Coverage 88.82% 88.66% -0.16%
==========================================
Files 4 4
Lines 1047 1059 +12
==========================================
+ Hits 930 939 +9
- Misses 117 120 +3 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
|
Thank you for this PR, could you kindly address the un-covered lines with additional tests? |
Sorry, something went wrong.
| """ | ||
| import types | ||
|
|
||
| class _FixedNow(datetime): |
There was a problem hiding this comment.
This is a pattern I'm not used to seeing. Could you explain to me what's going on here and how that ties into your assertions below?
Sorry, something went wrong.
There was a problem hiding this comment.
i think i get it now -- this forces datetime.now to always return the same 2026-03-05 date.
Sorry, something went wrong.
| code = "KUAO {}0053Z AUTO 31009KT 10SM -RA FEW046 BKN055 OVC070 05/00 A2980" | ||
|
|
||
| # The 31st: February has none, so the report is January's. | ||
| assert Metar.Metar(code.format(31)).time == datetime(2026, 1, 31, 0, 53) |
There was a problem hiding this comment.
we use pytest. can you parametrize these?
Sorry, something went wrong.
|
@akrherz one concern I have if that if we don't really know the month, how often is the assumption to use the first previous valid month correct? What are the situations in which folks would encounter this? (I'm always parsing METAR codes from ASOS data, which has the date bundled separately) |
Sorry, something went wrong.
There is no right answer to this question. I do like what this PR intends to do. In general, I think the user needs to apply some QC logic to whatever timestamp this library generates to know if it should be "trusted" or not. They should have some processing context to know about when a given observation is valid, shrug. |
Sorry, something went wrong.
Review asked for the uncovered lines. Looking at what they were showed the loop was unnecessary: every 30-day month and February is preceded by a 31-day month, so the inferred month never needs to walk back more than once. The bounded loop, the year rollback and the year_was_given guard were all unreachable for a day of 01-31, which is exactly why coverage could not reach them. One conditional step back replaces them. Tests now check the invariant rather than asserting it, parsing every day of every month across a leap and a non-leap year, and cover both the guarded and unguarded paths.
|
@akrherz Done — and asking was the useful part. The uncovered lines turned out to be unreachable: every 30-day month and February is preceded by a 31-day month, so the walk never needs to pass January. The loop is gone, replaced by one conditional step back — 13 lines instead of 17, both branches covered. @phobson Fair question, and I don't think the guess can be shown "right". But it only fires where the old code raised: the condition is exactly the input that produced day is out of range for month, so every parse that works on main today is unchanged. The choice isn't between two guesses, it's between a best-effort date and an exception on input the library currently can't handle. Your ASOS case passes month/year explicitly and is short-circuited — there's a test asserting an explicit February plus a 31st still raises. |
Sorry, something went wrong.
akrherz
left a comment
There was a problem hiding this comment.
This looks good to me.
Sorry, something went wrong.
Fixes #167.
A METAR carries only the day of the month, so _handleTime infers the rest:
this month, or last month if the day is still ahead of today. When last month
does not have that day, the inferred date does not exist and
datetime.datetime(...) raises:
The failure is calendar-dependent, which is why it comes and goes: reading a
30th or 31st report during the first days of March, May, July, October or
December hits it, and the same code works fine a week later.
Change: when the month was inferred, walk back to the most recent month
that actually has that day, instead of stepping back exactly one. The loop is
bounded at 12 iterations because day comes from a \d\d group and can be
nonsense.
An explicit month= is left alone. If a caller asserts February and passes a
31st, that is a real error and still raises — only the guess is softened.
Tests: two added, test_issue167_*. They pin the clock via monkeypatch
so they do not pass or fail depending on the day they are run. The first fails
on main with the traceback above; the second guards the explicit-month path.
Full suite: 73 passed.
Happy to add a CHANGELOG entry once this has a PR number, or to adjust the
walk-back if you would rather it only ever look one extra month back.