fix: a holiday named after a weekday answers with the holiday - #374
Draft
openvoiceos-bot wants to merge 1 commit into
Draft
openvoiceos-bot wants to merge 1 commit into
openvoiceos-bot wants to merge 1 commit into
Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Synchronizing... Check results have been successfully retrieved. 📡I've aggregated the results of the automated checks for this PR below. ⚖️ License CheckI've checked for any conflicting terms of service. 📜 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔨 Build TestsI've checked the welds on your new features. 👨🏭 ✅ All versions pass
📊 CoverageQuantifying the quality of our test suite. 🧪 Files below 80% coverage (32 files)
Full report: download the Automating the boring stuff so you don't have to! 🚀 |
JarbasAl
force-pushed
the
fix/holiday-shadowing-and-partial-match
branch
from
September 29, 2026 01:12
50b8699 to
351ad89
Compare
`extract_datetime` runs the per-language engine first and asks the holiday layer only when the engine read nothing. A holiday whose own name is written with the calendar vocabulary of its language — "good friday", "palm sunday", "ash wednesday" — is therefore read as that weekday, and the layer that knows the date is never asked. Eight of the twelve weekday- and month-named English holidays answered with the coming weekday: at an anchor of 25 September 2026, "good friday" answered 25 September, the anchor day, which is a Friday. The engine-first order is right everywhere else, so it is not reversed. `holiday_overrides_engine` asks one narrow question instead: are the words the engine consumed all words the holiday phrase itself covers? They are for "good friday", where the engine consumed "friday" and left "good". They are not for "play christmas music on friday", where the engine read a Friday the phrase "christmas" does not cover, and that answer stands. The question costs nothing on a text that names no holiday of its language: `_names_a_holiday` is a table lookup and answers first. Measured at the same anchor: eight of eight now answer with the holiday, "sunday" still answers with the coming Sunday, and "boxing day", which the engine cannot read at all, is unchanged. The review of this head asked for two things. The extent test compares a set of folded words, so a weekday word written both inside the holiday name and outside it cannot be told from one inside the name alone: the docstring now says so, and two known-answer cells hold the answers the library gives for "the friday before good friday" and "monday after easter monday" so the day the extent becomes positional the cells report the change. A leftover `if holiday is None: return None` below the line that returns `found` was unreachable and said the opposite of it; it is deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
JarbasAl
force-pushed
the
fix/holiday-shadowing-and-partial-match
branch
from
September 29, 2026 01:51
351ad89 to
d830809
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Finding 5 of reviewer-c's review of #369: a holiday whose own name carries the
calendar vocabulary of its language answers with a weekday instead of the
holiday.
extract_datetimeruns the per-language engine first and asks the holiday layeronly when the engine read nothing. "good friday" therefore reaches the engine,
which reads "friday" and answers with the coming Friday, and the holiday layer —
which knows the date — is never asked.
The engine-first order is right everywhere else and is not reversed.
holiday_overrides_engineasks one narrow question: are the words the engineconsumed all words the holiday phrase itself covers?
phrase. The holiday layer answers.
"christmas" does not cover. The engine's answer stands.
A text that names no holiday of its language never reaches chronologia:
_names_a_holidayis a table lookup and answers first.Measured, anchor 25 September 2026 (a Friday)
Every expected date is reckoned independently of the parser. Western Easter 2027
is 28 March by the Gregorian computus, and the movable feasts are counted from
it: Palm Sunday −7, Maundy Thursday −3, Good Friday −2, Holy Saturday −1, Easter
Monday +1, Whit Monday +50, Shrove Tuesday −47, Ash Wednesday −46.
origin/devUnchanged: "may day", "boxing day" and "earth day" already agreed; "labour day"
is declined by both sides.
Controls
holiday is untouched.
through the unchanged None path.
Cost
The extra question runs only when the text names a holiday of its language.
Measured in one process after the language spec is loaded: "good friday" 7.2 ms,
"play christmas music on friday" 3.9 ms, "remind me on friday" 9.5 ms (no
holiday word, so no chronologia call), "what is the weather" 0.7 ms.
extract_datetime_spanson "play some christmas music" is 54 ms, so finding 1's29.9 seconds stays fixed.
Failing-first
16 new assertions were written before the fix and all 16 failed on
origin/devat a9c7ff4 — eight dates and eight remainders. The four controls passed before
and after. Whole suite after: 2892 passed, 3 skipped, 14 xfailed, 1797 subtests.
The other three findings of that review are chronologia's
Measured on this same tree and settled by chronologia's own tables, not by
opinion. They are filed as separate tasks and are not touched here.
hebrew_new_yearconstruction has one order,article? hebrew new year_word YEAR, which requires a year slot. With noyear, nothing matches it, and
new_year_reftakes the "new year" substring.new_year_ref's own docstring says it is kept a separate construction so thatit "would [not] shadow
_resolve_hebrew_new_year", so the grammar contradictsits stated design. A year-less order is the fix.
no verb tense. The docstring half is already corrected on dev; whether a
date library should read a verb at all is a design question and is filed as a
panel decision.
CONFIRMED, where the review had it plausible.
chronologia.extract_timespan("thanksgiving", lang="en", anchor=REF, jurisdiction=j)returns 2026-11-26 for US, CA, GB, DE and None alike. Thewell-known table's own comment says the surface is offered "only under the
documented U.S. reading" and that a Canadian reference is "NOT silently
resolved to the U.S. date" — which is what happens.
ca.tabalready carriesthe Canadian rule as
nth_weekday | Thanksgiving Day | 10 2 0.Evidence:
knowledge/wiki/audits/chronologia/t5128-date-parser-369-findings.md🤖 Generated with Claude Code