From 844e67a7a2b51e57d81908c26467b744f142acff Mon Sep 17 00:00:00 2001 From: JarbasAi Date: Sat, 26 Sep 2026 00:19:28 +0000 Subject: [PATCH] fix: a holiday named after a weekday answers with the holiday MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. Second round, reviewer-c's CONFIRMED 1 at d830809. The first cut compared the words the engine consumed against the words of the holiday phrase and declined whenever the engine consumed anything else. A time of day is something else, so "good friday at 9am" and five siblings kept the engine's coming weekday and left half the holiday's own name in the remainder: 'good', 'palm', 'easter', 'shrove'. Every parametrisation of the invariant cell was a bare holiday name, so none of these was covered. holiday_overrides_engine now asks the engine the holiday phrase on its own and fires when that answer's date equals the date the engine gave the whole text. A time of day does not move the date, so the six override as the bare name does; "good friday and next friday" and "play christmas music on friday" still keep the engine's answer, because their dates differ or the slice reads nothing. It takes the engine's datetime in place of its remainder, which is a signature change to a function this same pull request introduces. Only the date is compared. The holiday layer answers with a date, so the engine's time of day is not carried onto it: "good friday at 9am" gives Good Friday at midnight and leaves "at 9am" in the remainder. A known-answer cell pins that, because carrying the time would also have to take the time words out of the remainder, which is a second change this round does not make. A cheap guard runs before the second engine call: when the words the engine consumed share no word with the phrase, the call is skipped. It answers W1. It is semantically neutral, and "play christmas music on friday" costs 3.21 ms against the first cut's 3.95 ms, while "good friday at 9am" costs 8.47 ms against 3.69 ms, inside the 10 ms warm bound. 13 cells are added and shown to bite: restoring the subset comparison reds 13, forcing the guard to decline always reds 31, and disabling the guard reds none. Whole suite 2926 passed, 3 skipped, 14 xfailed, 2294 subtests, which also corrects W2's 2892. T-7225. Co-Authored-By: Claude Opus 5 --- ovos_date_parser/__init__.py | 20 ++- ovos_date_parser/holidays.py | 97 +++++++++++++++ test/test_holidays_chronologia.py | 198 +++++++++++++++++++++++++++++- 3 files changed, 311 insertions(+), 4 deletions(-) diff --git a/ovos_date_parser/__init__.py b/ovos_date_parser/__init__.py index 04a0cb5..fad5e9b 100644 --- a/ovos_date_parser/__init__.py +++ b/ovos_date_parser/__init__.py @@ -29,6 +29,7 @@ from ovos_date_parser.scoped_en import extract_scoped_date_en, SCOPED_VOCAB_EN from ovos_date_parser.holidays import (extract_holiday_date, extract_holiday_span, + holiday_overrides_engine, holiday_surfaces) from ovos_date_parser.eras_en import extract_era_date_en, ERA_PATTERNS_EN from ovos_date_parser.eras_pt import extract_era_date_pt, ERA_PATTERNS_PT @@ -412,15 +413,28 @@ def extract_datetime( reads no date, :func:`ovos_date_parser.holidays.extract_holiday_span` asks chronologia, and its answer is taken only when chronologia says a holiday construction is what matched. An utterance the engine already - reads keeps the engine's answer, so nothing that worked before changes. + reads keeps the engine's answer, with one exception: a holiday whose own + name is written with calendar vocabulary ("good friday", "palm sunday") + is read by the engine as that weekday, so when the engine answers the + holiday phrase on its own with the same date it gave the whole text + (:func:`~ovos_date_parser.holidays.holiday_overrides_engine`) the holiday + layer answers instead. A time of day, an ordinal or any other word beside + the holiday name does not stop this, because it does not move the date. + A date the engine read from a calendar word the phrase does not cover is + a different date, and stays the engine's. """ found = _extract_datetime_engine(text, lang, anchorDate=anchorDate, default_time=default_time) - if found is not None: + if found is not None and not holiday_overrides_engine( + text, lang, found[0], found[1], anchorDate=anchorDate): return found holiday = extract_holiday_span(text, lang, anchorDate=anchorDate) if holiday is None: - return None + # one return for both ways of arriving here: the engine read nothing + # and ``found`` is None, which is the old contract, or the override + # sent an engine answer to a holiday layer that then declined, and + # the engine's answer stands. + return found moment, remainder = holiday if default_time is not None: moment = moment.replace(hour=default_time.hour, diff --git a/ovos_date_parser/holidays.py b/ovos_date_parser/holidays.py index 6e6b51b..3595648 100644 --- a/ovos_date_parser/holidays.py +++ b/ovos_date_parser/holidays.py @@ -334,6 +334,103 @@ def extract_holiday_span(text: str, lang: str, return start, remainder.strip() +def holiday_overrides_engine(text: str, lang: str, engine_date: datetime, + engine_remainder: str = "", + anchorDate: Optional[datetime] = None) -> bool: + """Whether a language engine read its date out of the holiday phrase. + + A holiday name may be written with calendar vocabulary of its own + language: "good friday", "palm sunday", "may day". A per-language engine + reads the weekday inside such a name and answers with the coming Friday, + and the holiday layer, asked only when the engine found nothing, is never + reached. The engine order is right everywhere else, so the question asked + here is narrow: did the engine take its date from the holiday phrase + itself? + + Args: + text: the text the engine was given. + lang: the BCP-47 code the engine was given. + engine_date: the datetime the engine answered with. Its date is the + value compared; its time is not read. + engine_remainder: what the engine left over. Read only by the cheap + guard below, which declines without a second engine call when the + engine consumed no word of the holiday phrase. + anchorDate: the date relative dating is reckoned from. + + The question is answered by asking the engine the holiday phrase on its + own. The override fires when that answer's date equals ``engine_date``, + the date the engine gave the whole text. For "good friday" the slice + "good friday" gives the coming Friday and so does the whole text, so the + engine read its date out of the phrase. For "play christmas music on + friday" the slice is "christmas", which the engine cannot read at all, so + the Friday came from elsewhere and stays the engine's. For "good friday + and next friday" the slice gives the coming Friday and the whole text + gives the one after, so the dates differ and the engine keeps its answer. + + Only the date is compared, never the time. A time of day beside the + holiday name does not move the date, so "good friday at 9am" and + "remind me on good friday at 9am" override exactly as the bare name does. + That is this function's reason for existing in this shape: the first cut + compared the words the engine consumed against the words of the phrase + and declined whenever the engine consumed anything else, which left half + the holiday's name in the remainder on the commonest spoken form of all + eight of these holidays. + + The holiday layer answers with a date, so the engine's time of day is not + carried onto it. "good friday at 9am" gives Good Friday at midnight and + leaves "at 9am" in the remainder. + + A date word that moves the date keeps the engine's answer, which is the + right outcome for "good friday and next friday" and the wrong one for + "the friday before good friday": the slice and the whole text both give + the coming Friday there, so the override fires and the answer is Good + Friday rather than the Friday before it. "monday after easter monday" + is the same shape. Both were already wrong before this function existed, + where the engine gave the coming weekday, so this is a bound on the fix + and not a regression. To decide it properly, compare character offsets + against the extent ``(first, last)`` computed below. + ``test_holidays_chronologia.py`` carries a cell for each known answer, so + the day the extent becomes positional the cell says what changed. + + The engine is asked twice for a text that names a holiday of its + language, once by the caller and once here, so such a text costs more + than one that names none. A text that names no holiday pays nothing: the + table lookup below answers first. + + A text that names no holiday of its language never reaches chronologia: + :func:`_names_a_holiday` is a table lookup and answers first. + """ + if not text: + return False + if not _names_a_holiday(text, lang): + return False + anchor = anchorDate or datetime.now() + written = _as_written(text, lang) + extent = _holiday_extent(written, lang, anchor) + if extent is None: + return False + first, last = extent + phrase_words = _word_set(written[first:last]) + consumed = _word_set(text) - _word_set(engine_remainder) + if not (consumed & phrase_words): + # The engine read its date from words the phrase does not cover, so + # the slice below cannot give the same date except by coincidence. + # Declining here is what keeps a text that merely names a holiday + # ("play christmas music on friday") to one engine call. + return False + # imported here, not at module scope: ovos_date_parser/__init__.py imports + # this module, so a top-level import would be a cycle. + from ovos_date_parser import _extract_datetime_engine + inner = _extract_datetime_engine(written[first:last], lang, + anchorDate=anchor) + return inner is not None and inner[0].date() == engine_date.date() + + +def _word_set(text: str) -> FrozenSet[str]: + """The folded words of ``text``, for comparing one extent with another.""" + return frozenset(_fold(word) for word in re.findall(r"\w+", text or "")) + + def extract_holiday_date(text: str, lang: str, ref_date: Optional[date] = None ) -> Optional[Tuple[date, str]]: diff --git a/test/test_holidays_chronologia.py b/test/test_holidays_chronologia.py index 8a873cc..c12c212 100644 --- a/test/test_holidays_chronologia.py +++ b/test/test_holidays_chronologia.py @@ -8,7 +8,7 @@ The anchor is fixed at 25 September 2026, so "the next one" is a stated date rather than whatever today makes it. """ -from datetime import date, datetime +from datetime import date, datetime, time import pytest @@ -227,3 +227,199 @@ def test_a_tense_inside_the_holiday_phrase_still_reads(utterance, expected): "christmas eve" each as one match over all their words. """ assert extract_datetime(utterance, "en-US", REF)[0] == expected + + +# --- finding 5 of the #369 review: a holiday named after a weekday ---------- + +# `extract_datetime` runs the per-language engine first. A holiday whose own +# name carries weekday vocabulary — "good friday", "palm sunday" — therefore +# answered with the coming weekday and never reached the holiday layer, which +# knew the right date all along. Eight of the twelve weekday- and month-named +# English holidays answered that way. +# +# Every date below 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 is Easter minus 7 (21 March), Maundy Thursday minus 3 +# (25 March), Good Friday minus 2 (26 March), Holy Saturday minus 1 (27 March), +# Easter Monday plus 1 (29 March), Whit Monday plus 50 (17 May). Shrove Tuesday +# is Easter minus 47 (9 February) and Ash Wednesday minus 46 (10 February). + +WEEKDAY_NAMED_HOLIDAYS = [ + ("palm sunday", date(2027, 3, 21)), + ("maundy thursday", date(2027, 3, 25)), + ("good friday", date(2027, 3, 26)), + ("holy saturday", date(2027, 3, 27)), + ("easter monday", date(2027, 3, 29)), + ("whit monday", date(2027, 5, 17)), + ("shrove tuesday", date(2027, 2, 9)), + ("ash wednesday", date(2027, 2, 10)), +] + + +@pytest.mark.parametrize("utterance,expected", WEEKDAY_NAMED_HOLIDAYS) +def test_a_weekday_named_holiday_answers_with_the_holiday(utterance, expected): + """The holiday layer's date wins when the engine read the holiday's own + words as a weekday.""" + got = extract_datetime(utterance, "en-US", REF) + assert got is not None + assert got[0].date() == expected + + +@pytest.mark.parametrize("utterance,expected", WEEKDAY_NAMED_HOLIDAYS) +def test_a_weekday_named_holiday_keeps_no_half_of_its_name(utterance, expected): + """The remainder holds no word of the holiday's own name.""" + got = extract_datetime(utterance, "en-US", REF) + assert got is not None + for word in utterance.split(): + assert word not in got[1].lower() + + +# --- a word of time beside the holiday name, from the #374 review ---------- + +# The first cut compared the words the engine consumed against the words of the +# holiday phrase, and declined whenever the engine consumed anything else. A +# time of day is something else, so "good friday at 9am" kept the engine's +# coming Friday and left `'good'` in the remainder: half the holiday's own name, +# on the commonest spoken form of all eight. The test asks the engine the +# holiday phrase on its own instead, and the override fires when that answer's +# date equals the date the whole text produced, which a time of day does not +# move. +# +# Each expected date is reckoned independently from the computus, as above. + +WEEKDAY_NAMED_HOLIDAYS_WITH_A_TIME = [ + ("good friday at 9am", date(2027, 3, 26)), + ("remind me on good friday at 9am", date(2027, 3, 26)), + ("palm sunday at noon", date(2027, 3, 21)), + ("easter monday morning", date(2027, 3, 29)), + ("shrove tuesday at 6pm", date(2027, 2, 9)), + ("set an alarm for good friday at 7", date(2027, 3, 26)), +] + +WEEKDAY_NAMED_HOLIDAY_WORDS = frozenset( + word for utterance, _ in WEEKDAY_NAMED_HOLIDAYS for word in utterance.split() +) + + +@pytest.mark.parametrize("utterance,expected", + WEEKDAY_NAMED_HOLIDAYS_WITH_A_TIME) +def test_a_time_beside_a_holiday_name_still_answers_with_the_holiday( + utterance, expected): + """A time of day beside the holiday name does not send the answer back to + the engine's weekday.""" + got = extract_datetime(utterance, "en-US", REF) + assert got is not None + assert got[0].date() == expected + + +@pytest.mark.parametrize("utterance,expected", + WEEKDAY_NAMED_HOLIDAYS_WITH_A_TIME) +def test_a_time_beside_a_holiday_name_keeps_no_half_of_the_name( + utterance, expected): + """The remainder holds no word of any weekday-named holiday. + + The whole set is checked, not only the words of this utterance, because the + failure this cell exists for left `'good'`, `'palm'`, `'easter'` and + `'shrove'` behind. + """ + got = extract_datetime(utterance, "en-US", REF) + assert got is not None + remainder_words = set(got[1].lower().split()) + assert not (remainder_words & WEEKDAY_NAMED_HOLIDAY_WORDS) + + +def test_a_time_beside_a_holiday_name_is_left_in_the_remainder(): + """Known answer, and the bound on this round: the holiday layer answers + with a date, so the engine's time of day is not carried onto it. + + "good friday at 9am" gives Good Friday at midnight and leaves "at 9am" in + the remainder. The time is not lost, but a caller that reads only the + datetime sees midnight. Carrying it would also have to take the time words + out of the remainder, which is a second change this round does not make. + The day the time is carried this cell fails and says so. + """ + got = extract_datetime("good friday at 9am", "en-US", REF) + assert got is not None + assert got[0].date() == date(2027, 3, 26) + assert got[0].time() == time(0, 0) + assert "9am" in got[1] + + +def test_a_plain_weekday_still_answers_with_the_weekday(utterance=None): + """Control: a weekday that names no holiday is untouched. + + 25 September 2026 is a Friday, so the coming Sunday is the 27th. + """ + got = extract_datetime("sunday", "en-US", REF) + assert got is not None + assert got[0].date() == date(2026, 9, 27) + + +def test_a_weekday_beside_a_holiday_keeps_the_weekday(): + """Control: the engine's answer stands when it read words the holiday + phrase does not cover. + + "play christmas music on friday" asks about Friday. The holiday phrase is + "christmas" alone, so the engine's Friday is not inside it and the holiday + date must not replace it. + """ + got = extract_datetime("play christmas music on friday", "en-US", REF) + assert got is not None + assert got[0].date() == date(2026, 9, 25) or got[0].date() == date(2026, 10, 2) + assert got[0].date() != date(2026, 12, 25) + + +def test_a_holiday_the_engine_cannot_read_is_unchanged(): + """Control: the None path of the engine still answers from the layer.""" + got = extract_datetime("boxing day", "en-US", REF) + assert got is not None + assert got[0].date() == date(2026, 12, 26) + + +# --- the bound on the extent test, from the #374 review --------------------- + +# `holiday_overrides_engine` compares a set of folded words, not a position, so +# a weekday word that appears both inside the holiday name and elsewhere in the +# sentence is indistinguishable from one that appears inside the name alone. A +# relative phrase built on such a word answers with the holiday itself. +# +# These cells record the answer this library gives today, not the answer the +# phrase asks for. Each expected date is reckoned independently: Western Easter +# 2027 is 28 March by the Gregorian computus, so Good Friday is 26 March and +# Easter Monday is 29 March. The date the phrase actually names is written +# beside each cell. The day the extent test becomes positional these cells fail +# and say what changed. + +KNOWN_ANSWER_RELATIVE_PHRASES = [ + # utterance, the date given today, the date the phrase names + ("the friday before good friday", date(2027, 3, 26), date(2027, 3, 19)), + ("monday after easter monday", date(2027, 3, 29), date(2027, 4, 5)), +] + + +@pytest.mark.parametrize("utterance,given,asked", + KNOWN_ANSWER_RELATIVE_PHRASES) +def test_a_relative_phrase_on_a_holiday_word_answers_with_the_holiday( + utterance, given, asked): + """Known answer: the holiday's own date, not the day the phrase names. + + The engine gave the coming weekday for these phrases before this fix + existed, which was wrong too, so the cell bounds the fix rather than + approving it. + """ + got = extract_datetime(utterance, "en-US", REF) + assert got is not None + assert got[0].date() == given + assert got[0].date() != asked + + +def test_a_relative_phrase_the_engine_keeps_is_not_taken(): + """Control, the opposite call on a phrase of the same shape. + + "good friday and next friday" carries a Friday outside the holiday phrase, + so the consumed words are not a subset of the phrase and the engine keeps + the utterance. 25 September 2026 is a Friday, so the next one is 2 October. + """ + got = extract_datetime("good friday and next friday", "en-US", REF) + assert got is not None + assert got[0].date() == date(2026, 10, 2)