Repository navigation
Parse last, this and next before a weekday - #1401
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1401 +/- ##
==========================================
+ Coverage 98.97% 99.02% +0.04%
==========================================
Files 239 239
Lines 3627 3786 +159
==========================================
+ Hits 3590 3749 +159
Misses 37 37 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
serhii73
left a comment
There was a problem hiding this comment.
Thanks, last/this/next <weekday> works well with parse(). I found two problems before this can go in:
1. search_dates() without languages now loses these dates.
from datetime import datetime
from dateparser.search import search_dates
search_dates("Deadline: next Friday")
# master: [('Friday', ...)] this PR: None
search_dates("I will be there next Tuesday.")
# master: [('Tuesday', ...)] this PR: [('be', datetime(2026, 11, 30, 0, 0))]
search_dates("See you next Friday at 5pm or last Monday",
settings={"RELATIVE_BASE": datetime(2020, 6, 15, 12)})
# master: [('Friday at 5pm', ...), ('Monday', ...)]
# this PR: [('See', datetime(2020, 4, 15, 0, 0)), ('5pm', ...)]The English splitter now keeps next tuesday as one token. But
Locale._count_words_present_in_the_dictionary() looks tokens up in the split dictionary, which
only has next and tuesday. So the phrase is not counted, English scores 0 and language
detection picks Yoruba/Basque/Fula (DateSearchWithDetection().detect_language(text, None)
returns yo, eu, ff for the texts above). One possible fix: count a multi-word token when all
its words are in the split dictionary. That fixed all three texts in a quick local check. It would
also be good to have a search test without languages=["en"].
2. Bare weekdays change under the default settings.
from datetime import datetime
import dateparser
mon = {"RELATIVE_BASE": datetime(2020, 6, 15, 12)} # a Monday
dateparser.parse("Friday", settings=mon) # master: 2020-06-12 this PR: 2020-06-19
dateparser.parse("vendredi", settings=mon) # master: 2020-06-12 this PR: 2020-06-19
dateparser.parse("Sunday", settings={**mon, "DEFAULT_START_OF_WEEK": "sunday"})
# this PR: 2020-06-21, outside a Sunday-started current week (Jun 14-20)I understand this is what #1031 asks for, but it changes the default result for every bare weekday
in every language. For example, "Posted Friday at 10:15" in scraped text now becomes a future date
early in the week, and users have no way to get the old behaviour back. Could we move the last
commit (bare weekday within the current week) to a separate PR, ideally behind an opt-in, so this
one only adds the new last/this/next support? If the change stays, the "current week" should
probably follow DEFAULT_START_OF_WEEK.
serhii73
left a comment
There was a problem hiding this comment.
Requesting changes for one thing; otherwise this looks very good.
I reviewed the exact merge result (this head merged into current master; it merges cleanly). Fine:
last/this/next <weekday>matches an independent oracle in about 24,000 checks (all weekday names and abbreviations, every base weekday, everyPREFER_DATES_FROMvalue, time zone and DST bases), also12am last mondayand times.- Bare weekdays and the existing phrases (
next week,last year,this morning, ...) are identical to master in about 100,000 comparisons over 205 locales and both week starts. - Both problems of my earlier review are fixed: the three
search_dates()texts returnnext Friday,next Tuesday, andnext Friday at 5pm+last Monday, and bare weekdays no longer change. - The full suite passes on the merge result (24782 passed); mypy strict, ruff, pre-commit, the oldest dependencies and Python 3.10 to 3.14 are green.
Blocker: the _count_words_present_in_the_dictionary change misdetects ordinary dates when languages is not given.
search_dates("11 Mai 2014") # master: [('11 Mai 2014', 2014-05-11)] (fr)
# here: [('2014', <today>)] (detected as vi)
search_dates("Thu 12 February 2016 11:41") # master: en, 2016-02-12 11:41 here: vi, a wrong date
search_dates("il 5 maggio 2026") # master: it, 2026-05-05 here: fr, a wrong date
search_dates("11 mai") # master: [('11 mai', ...)] here: NoneThe splitter leaves padded, unrecognised chunks such as " mai ". " mai ".split() is ["mai"], and mai is in the split dictionary of Vietnamese only as a part of ngày mai, so Vietnamese scores one more and wins (dict_cnt is compared first). The same happens with le, il, till and de. CI cannot see it, because no test runs search_dates without languages on short non-English dates. On a corpus of 22,956 texts, 3,842 get a better language and 60 a worse one; on 12,858 plain dates in 184 languages there are 71 regressions and no improvement.
The fix is to count the parts of a token only when the token is itself a dictionary entry that the splitter kept whole, which is the case this change is for (next friday):
entries = self._get_dictionary(settings)
...
if len(parts) > 1 and word in entries and all(part in dictionary for part in parts):With it your next Friday / next Tuesday texts stay fixed, the plain-date sweeps have no difference from master, and the suite passes (24787 with the new tests, which fail without the fix).
Patch with regression tests (git apply)
diff --git a/dateparser/languages/locale.py b/dateparser/languages/locale.py
index ee068dc6..72bf61f0 100644
--- a/dateparser/languages/locale.py
+++ b/dateparser/languages/locale.py
@@ -133,6 +133,7 @@ class Locale:
dictionary = self.clean_dictionary(
self._get_split_dictionary(settings=settings)
)
+ entries = self._get_dictionary(settings)
dict_cnt = 0
skip_cnt = 0
# The splitter keeps multi-word dictionary entries, e.g. "next friday",
@@ -140,7 +141,11 @@ class Locale:
split_words = set()
for word in words:
parts = word.split()
- if word not in dictionary and all(part in dictionary for part in parts):
+ if (
+ len(parts) > 1
+ and word in entries
+ and all(part in dictionary for part in parts)
+ ):
split_words.update(parts)
else:
split_words.add(word)
diff --git a/tests/test_search.py b/tests/test_search.py
index 404afaa9..36083f9e 100644
--- a/tests/test_search.py
+++ b/tests/test_search.py
@@ -1274,6 +1274,55 @@ class TestTranslateSearch(BaseTestCase):
)
self.assertEqual(result, expected)
+ @parameterized.expand(
+ [
+ param(
+ text="I will be there next Tuesday.",
+ expected=[("next Tuesday", datetime.datetime(2026, 9, 29), "en")],
+ ),
+ param(
+ text="11 Mai 2014",
+ expected=[("11 Mai 2014", datetime.datetime(2014, 5, 11), "fr")],
+ ),
+ param(
+ text="Le 11 Décembre 2014 à 09:00",
+ expected=[
+ (
+ "Le 11 Décembre 2014 à 09:00",
+ datetime.datetime(2014, 12, 11, 9),
+ "fr",
+ )
+ ],
+ ),
+ param(
+ text="il 5 maggio 2026",
+ expected=[("5 maggio 2026", datetime.datetime(2026, 5, 5), "it")],
+ ),
+ param(
+ text="след 12 мин 18 сек",
+ expected=[
+ (
+ "след 12 мин 18 сек",
+ datetime.datetime(2026, 9, 23, 12, 12, 18),
+ "bg",
+ )
+ ],
+ ),
+ ]
+ )
+ def test_search_dates_language_detection_counts_only_whole_entries(
+ self, text: str, expected: list[tuple[str, datetime.datetime, str]]
+ ) -> None:
+ # Only the words of a multi-word dictionary entry that the splitter keeps
+ # whole count for language detection, not those of an unrecognised chunk
+ # such as " mai " (the "mai" of Vietnamese "ngày mai").
+ result = search_dates(
+ text,
+ settings={"RELATIVE_BASE": datetime.datetime(2026, 9, 23, 12)},
+ add_detected_language=True,
+ )
+ self.assertEqual(result, expected)
+
@parameterized.expand(
[
param(PR description
fixes #1031would close an issue this PR does not fix: the bare-weekday change was reverted, andFridaywithcurrent_periodgives the same result on master and here, which is the issue's own example.fixes #898:12am last monday,last mondayandthis mondaywork, but two lines of that issue's own script (last week on Monday,a week ago on Monday) still returnNone, so it ispart of #898.- Suggested: "Fixes #573, fixes #635, fixes #1177, part of #898, part of #1171", and a hand-written squash message, because the default one would list the commit that was reverted.
Non-blocking follow-ups
- Other languages have no
last/this/next <weekday>entries and the README does not say it is English-only. Nothing will track that once #573 closes. I checked that sevenrelative-typeentries infr.yamlmake the Frenchlundi prochainexamples work on top of this PR (see #1536). - A modifier is ignored silently when the weekday is not applied (
last friday of may,next friday junenow give a date where master gaveNone), and a bare modifier that reaches the parser (DEFAULT_LANGUAGES, the Hijri/Jalali/Minguo parsers) is swallowed instead of raising; a two-line guard in the token loop would fix the second. - In
search_dates, a comma or)right after the weekday drops the modifier (same mechanism as on master), and a sentence listing several modified weekdays can return fewer dates than master (Meet next friday, last monday and next sunday). - #1435 overlaps with this change in
locale.py, and 7 other open PRs will conflict trivially once this merges.
serhii73
left a comment
There was a problem hiding this comment.
Requesting changes again. The language-detection blocker is fixed, but search_dates still loses or changes dates that master gets right: some of that comes from the last two commits, and some was already there at 5ce0071a.
I reviewed the exact merge result (9a3d1ca already contains current master, fed9cf9). Fine:
- The blocker of my last review is fixed as suggested:
11 Mai 2014,Thu 12 February 2016 11:41,il 5 maggio 2026and11 maigive the same language and dates as master. parse()withlast|this|next <weekday>equals an independent oracle (written from the README) in all 28,224 cells of a matrix: every weekday name, 8 windows of 14 consecutive bases with month and year boundaries and leap days, the 3PREFER_DATES_FROMvalues, both week starts, with and withoutlanguages. Bare weekdays are identical to master in all 28,560 cells of a similar matrix.search_dates()with a single modified weekday works now where master drops the modifier (the three texts of my first review, for instance). The suite passes (24,663 here) and CI is green.
Blockers. All three are in search_dates. Blockers 1 and 3 need no special setting; blocker 2 only shows without RELATIVE_BASE, and all but one of the added search tests pass it (the exception starts with an explicit date, which supplies the base). None of the added tests has a modifier phrase next to an explicit date, a modifier phrase joined with and or / (not or or a comma) to a relative expression (yesterday, today) or to a phrase with a time, or a (next week)-style phrase right after a date. Calls use languages=["en"] and RELATIVE_BASE=datetime(2020, 6, 15, 12) (a Monday) unless noted; every result below is the same without languages. "master" is fed9cf9. In the code blocks, # since X means that master's result is right and X is the first of the PR states I ran (5ce0071a, 2cdab68, 9a3d1ca) where it is not; already at 5ce0071a means that all three are wrong. The dates that master returns in these examples are right (its substrings are sloppy, e.g. 'Sunday at 5pm and'). The examples use an explicit date, which decides the result on master, or last, because master ignores the modifier and a bare weekday means the past; with a bare this or next weekday master is wrong, and those are not regressions.
1. A last|this|next <weekday> phrase glues itself to the date next to it
next Friday 19/06/2020 # since 2cdab68
master [('Friday 19/06/2020', 2020-06-19)]
head [('Friday', 2020-06-12), ('19/06/2020', 2020-06-19)] modifier dropped, wrong date
Join us this Saturday 20 June at 8pm # since 2cdab68
master [('Saturday 20 June at 8pm', 2020-06-20 20:00)]
head None
I was there last Sunday at 5pm and last Friday at 5pm # already at 5ce0071a
master [('Sunday at 5pm and', 2020-06-14 17:00), ('Friday at 5pm', 2020-06-12 17:00)]
head None
The case was heard yesterday and last Friday. # already at 5ce0071a
master [('yesterday and', 2020-06-14 12:00), ('Friday', 2020-06-12)]
head None
3 June 2021 / last Friday at 10:30 # already at 5ce0071a (as one merged date)
master [('3 June 2021 /', 2021-06-03), ('Friday at 10:30', 2020-06-12 10:30)]
head [('3 June 2021', 2021-06-03), ('/ last Friday', 2020-06-12), ('10:30', 2020-06-15 10:30)]
Fireworks are next Friday, July 4, 2025. # since 9a3d1ca
master [('Friday, July 4, 2025', 2025-07-04)]
head [('next Friday', 2020-06-19), ('July 4', 2020-07-04), ('2025', 2025-06-15)]
The shop was shut today and last Friday, sadly. # since 9a3d1ca
master [('today and', 2020-06-15 12:00), ('Friday', 2020-06-12)]
head None
Cause: last friday and its siblings are now two-word dictionary entries, so translate_search keeps them in the same chunk as their neighbours. On master the unknown word last ended the chunk, which is why the neighbouring date was parsed on its own in these examples. Since 2cdab68 the parser rejects a modifier next to an explicit day, month or year (elif self._weekday_modifier: raise ValueError), which is right for parse(), so the whole chunk fails, and in these examples none of the existing splitters can cut it where the modifier starts: they split into uniform 1/2/3-word groups, and split_around_skipped_words gives up as soon as one word translates to several (5pm becomes 5 pm). What is left are fragments, and a bare 2020 parses as a date against the base. 9a3d1ca makes the pair lookup ignore surrounding punctuation (_in_dictionary), so last friday, with a comma joins its neighbours too (the last two examples). The same inputs without a modifier already fail on master (Today and Friday, I was ill. gives None); with last they worked because it cut the chunk.
2. Without RELATIVE_BASE, last X and last Y comes out a week early (wrong dates since the merge of master into the branch, 22c7753c; 5ce0071a predates it and returned None)
last Monday and last Friday (today is Wed 2026-10-07)
master [('Monday and', 2026-10-05), ('Friday', 2026-10-02)]
head [('last Monday', 2026-09-28), ('last Friday', 2026-09-25)]
Cause: parse_item replaces parser._settings with a copy that has the previous date as RELATIVE_BASE and never puts it back. last monday last friday is now one chunk with several candidate splits, scored one after another on the same parser, so the base found while scoring the first candidate leaks into the first date of the second. With RELATIVE_BASE given that branch is skipped, which is why the added tests pass. The offset is the same on any day. The leak is on master too (Monday 10:00 and Wednesday 11:00 has its first date a week early), but last X and last Y never reached it there, because last ended the chunk. On 441 sentences M1 W1 and M2 W2 (M is last, this or next, W a weekday), with now() pinned to each day of a week (3,087 cases), master gets the first date right in 1,029 and the head in 1,323; but in 882 of the 1,029 that master got right, the head is exactly a week early (it is better on the others, because this and next are supported now).
This one has a small fix: put the settings back in a finally, as below, with two tests that pin the clock with patch("dateparser.parser._now", ...) (freezegun is not used in this repo). With RELATIVE_BASE given that branch is skipped, so nothing changes. Without RELATIVE_BASE the first date is right in all 3,087 cases above with the fix, and some texts without a modifier that depended on the leak are corrected too (Monday 10:00 and Wednesday 11:00). One side effect I know of: when the splitter cuts the year off an explicit date and another item follows, the first fragment used to get its year back only through the leaked base. 3 Mar 2019 and June 5 gives ('3 Mar', 2019-03-03), ('2019 and June', 2019-06-03) on master and on the head; with the fix 3 Mar gets the year of now (2026-03-03), which is what all three give when RELATIVE_BASE is given (the year of the base). In generated families of pairs, triples, times, time zones, settings, brackets and four other languages (169,000 runs, now() pinned to every weekday) the fix makes the first date right in 6.4% more of the runs and wrong in 0.45%. The suite passes (24,665, and the two new tests fail on the head) and ruff check dateparser tests and strict mypy are clean.
Fix for 2, with tests (git apply, on top of 9a3d1ca)
diff --git a/dateparser/search/search.py b/dateparser/search/search.py
index af5effd..bc0599c 100644
--- a/dateparser/search/search.py
+++ b/dateparser/search/search.py
@@ -272,8 +272,14 @@ class _ExactLanguageSearch:
item, relative_base = self.set_relative_base(item, parsed)
if relative_base:
- parser._settings = parser._settings.replace(RELATIVE_BASE=relative_base)
- parsed_item = parser.get_date_data(item)
+ settings = parser._settings
+ parser._settings = settings.replace(RELATIVE_BASE=relative_base)
+ try:
+ parsed_item = parser.get_date_data(item)
+ finally:
+ # Do not leave this base behind for the next call, which may
+ # belong to another candidate split.
+ parser._settings = settings
return parsed_item, is_relative
def parse_found_objects(
diff --git a/tests/test_search.py b/tests/test_search.py
index 55d0cdd..7716837 100644
--- a/tests/test_search.py
+++ b/tests/test_search.py
@@ -1,6 +1,7 @@
import datetime
from datetime import timedelta
from typing import Any
+from unittest.mock import patch
import pytz
from parameterized import param, parameterized
@@ -1440,6 +1441,35 @@ class TestTranslateSearch(BaseTestCase):
],
)
+ @parameterized.expand(
+ [
+ param(
+ text="last Monday and last Friday",
+ expected=[
+ ("last Monday", datetime.datetime(2026, 9, 21)),
+ ("last Friday", datetime.datetime(2026, 9, 18)),
+ ],
+ ),
+ param(
+ text="last Sunday and last Friday",
+ expected=[
+ ("last Sunday", datetime.datetime(2026, 9, 20)),
+ ("last Friday", datetime.datetime(2026, 9, 18)),
+ ],
+ ),
+ ]
+ )
+ def test_search_dates_weekday_with_modifier_without_relative_base(
+ self, text: str, expected: list[tuple[str, datetime.datetime]]
+ ) -> None:
+ # A candidate split must not leave the base of its first date behind for
+ # the next candidate: the first date is relative to now.
+ with patch(
+ "dateparser.parser._now", return_value=datetime.datetime(2026, 9, 23, 12)
+ ):
+ result = search_dates(text, languages=["en"])
+ self.assertEqual(result, expected)
+
@parameterized.expand(
[
param(3. An explicit date followed by a bracketed or quoted relative phrase is lost (since 9a3d1ca)
April 24 (next week)
master [('April 24', 2020-04-24)]
head None
5 Jan 2020 (last week)
master [('5 Jan 2020', 2020-01-05)]
head None
Monday, June 22 (next week) is the deadline.
master [('Monday, June 22', 2020-06-22)]
head [('Monday', 2020-06-15)]
2021-03-05 (next week)
master [('2021-03-05', 2021-03-05)]
head [('2021-03-05 (next week', 2020-06-22 12:00)] the date is replaced
The deadline is April 24, 2027 (next week). (today is Wed 2026-10-07, no RELATIVE_BASE)
master [('April 24, 2027', 2027-04-24)]
head [('April 24', 2026-04-24), ('2027 (next week', 2026-05-01)] wrong year
Cause: _in_dictionary and _translate_token now recognise (next week) as a two-word entry and append it to the chunk of the date before it (april 24 in 1 week)). That chunk does not parse, split_by_relative_expression skips it because there is text before and after the expression, and no other splitter lines up, so the date goes with it; for ISO and numeric dates the whole chunk parses as one item that carries the relative date (19/06/2020 (this week) gives 2020-06-15 12:00). On master (next was unrecognised and ended the chunk, so the date was parsed on its own (and the bracketed phrase ignored). It is not specific to English: for the two-word this|next|last week|month|year entries of every locale (about 4,200 sentences in 191 locales, 2021-03-05 (<entry>) and (<entry>) 2021-03-05), master finds the date in all but a few dozen and the head in almost none. Longer entries such as French la semaine prochaine are mostly unaffected. A bracketed single word or short phrase after a date breaks the same way (last two examples below).
These are families, not a complete list. More inputs that master gets right and the head does not, same settings:
this Saturday 20th # since 2cdab68
master [('Saturday 20th', 2020-06-20)]
head [('Saturday', 2020-06-13), ('20th', 2020-06-20)]
next Friday the 19th at 5pm # since 2cdab68
master [('Friday the 19th at 5pm', 2020-06-19 17:00)]
head None
last Tuesday Sept 6 / May 20 # already at 5ce0071a
master [('Tuesday Sept 6', 2020-09-06), ('/ May 20', 2020-05-20)]
head [('last Tuesday', 2020-06-09), ('Sept 6', 2020-09-06), ('/ May', 2020-05-15)]
The deadline (2020-06-19, next week) is firm. # since 9a3d1ca
master [('2020-06-19', 2020-06-19)]
head [('2020-06-19, next week', 2020-06-22 12:00)]
Event on 19/06/2020 (day 3) # since 9a3d1ca (None at 5ce0071a, fine at 2cdab68)
master [('19/06/2020', 2020-06-19)]
head [('on 19/06/2020 (day 3', 2014-12-04 12:00)]
19/06/2020 [month] # since 9a3d1ca
master [('19/06/2020', 2020-06-19)]
head [('19/06/2020 [month', 1852-02-15 12:00)]
I found all of these by running search_dates on generated sentences, master and the branch side by side, and counting where master's dates disappear or change; that is the way to find the rest.
What did not work. I would start in translate_search, where master got its accidental cut: end the chunk before a last|this|next <weekday> phrase and before a token that starts with a bracket or a quote. I do not have a complete fix, though: each prototype I tried (not attached) fixed the examples above and still broke sentences that the head handles. Things to watch for:
- Cutting a failed chunk at the modifier phrase inside
split_if_not_parsedand returning only that split bypasses the other splitters, and dropping the trailingandfrom the first part breaks their alignment:June 5 and yesterday and last Tuesday(three dates on master and on the head) lost two of them. - Reading a chunk that starts with a modifier again without the modifier (so that
next Friday 19/06/2020is the explicit date, as on master) makes candidate splits such aslast tuesday septparse and win the ranking (last Tuesday Sept 6 and May 20lost both dates). The retry has to ask for a whole date, and it has to keep the caller'sREQUIRE_PARTS(withREQUIRE_PARTS=['day', 'month', 'year'],next Friday 25 Septemberhas no year and must stayNone, as on master and on the head). - Ending the chunk after a bracketed entry splits
(last Friday at 5pm)(one date on master and on the head); it can only end after the entry that closes the bracket. - The bracket regression is not limited to two-word entries (see the list above), so a fix in the pair branch alone leaves the single-word variants.
PR description (as in my last review): the description still has fixes in front of #898 and #1031, which closes both on merge. Bare weekdays are identical to master, so the example of #1031 is unchanged, and the script of #898 still has two lines that return None (last week on Monday, a week ago on Monday), so it is part of #898. I would merge with a hand-written squash message, because the default one would list Resolve a bare weekday within the current week under current_period, which c3b16843 undid.
Non-blocking
- Without
RELATIVE_BASE, a modified weekday after the first date is evaluated against the previous date found, not against now:Meet this Friday or last Thursday(today Wed 2026-10-07) giveslast Thursday= 2026-10-08, which is tomorrow, where master gave 2026-10-01 for it. The README says "the current date", and the chaining is pinned bytest_search_dates_weekday_with_modifier_relative_to_previous_date, so I did not count it as a blocker of its own: the sentence as a whole is better than on master (this Fridayis now right). I would still evaluate the modifiers against now (the tests in the fix above use pairs where both agree); tell me if you see it differently. search_datesis slower on texts with modifier phrases (min of 7 runs on my machine):See you next Friday at 5pm or last Monday, and this Sunday, or maybe next Tuesday.takes 2.3 ms on master and 4.1 ms on the head (about 1.8 times), and a long text that mixes modifier phrases, brackets and dates takes two to four times longer. Plain dates, bare weekdays and texts without dates are unchanged.- Other languages have no entries and the README does not say the feature is English-only; nothing will track that once #573 closes.
Happy to look again once these are fixed.
serhii73
left a comment
There was a problem hiding this comment.
Thank you, the three blockers of my last review are fixed. One narrower problem is left, with the same cause as blocker 3, so I am still requesting changes.
I checked 52399c2, and its merge with current master (ac088237), which is clean and gives the same results.
Fixed
- Blocker 1:
next Friday 19/06/2020,Join us this Saturday 20 June at 8pm,I was there last Sunday at 5pm and last Friday at 5pmandFireworks are next Friday, July 4, 2025.now give master's dates. - Blocker 2: with
dateparser.parser._nowpinned to each day of a week,last Monday and last Fridayis right on all 7 days (it was a week early on all 7). - Blocker 3:
April 24 (next week),5 Jan 2020 (last week),2021-03-05 (next week),Event on 19/06/2020 (day 3)and19/06/2020 [month]give master's dates. - The full suite,
ruff, strictmypyand pre-commit pass.
Blocker: a relative expression in brackets or quotes after a connector makes the whole sentence return None
I compared master and the head on 82,176 generated English sentences (modifier pairs, explicit dates, relative expressions, times, connectors and/or/,///-/then, brackets on either side), scored against an independent oracle. Without brackets, nothing that master gets right is lost. With brackets, 344 sentences (0.4%) lose a correct date, and 336 of them return None. With languages=["en"] and RELATIVE_BASE=datetime(2020, 6, 15, 12) (same without languages):
last Friday at 5pm and (yesterday)
master [('Friday at', 2020-06-12 17:00), ('5pm and', 2020-06-14 12:00)]
head None
19/06/2020 and (tomorrow at 5pm)
master [('19/06/2020', 2020-06-19), ('5pm', 2020-06-15 17:00)]
head None
20 June 2020 / (in 3 days)
master [('20 June', 2020-06-20), ('2020 /', 2020-06-15)]
head None
The same happens with [...] and "...". last Friday at 5pm and yesterday (no brackets) works, so the bracketed expression seems to be glued to the chunk before it, and the chunk and its neighbouring date fail together. I have not tried a fix. Expected: at least what master returns; ideally the bracketed expression is read as it is without brackets. Danish 2021-03-05 (i år) shows the same thing (4 of 16,912 locale cases).
Not blocking
last|this|nextwork withmon,tue(s),thu,fribut not withwed,sat,sun,thur(s)(last wedisNone, as on master). If that is on purpose, a sentence in the docs would help.search_datesis about 1.2x slower on a text with modifier phrases (2.7 to 3.4 ms); texts without them are unchanged.- Not caused by this PR:
item.replace("am", "")inparse_itemmakes times like10amwrong insearch_dateson master (Friday at 10amgives 15 October). Probably an issue of its own.
Happy to look again once the bracket case is fixed.
serhii73
left a comment
There was a problem hiding this comment.
Thank you, the bracket blocker of my last review is fixed. One narrower regression is left, in the new code, so I am still requesting changes.
I checked 988b96f. It lacks only master's workflow-file commit (07b40b2f), so its merge with master is clean and the same.
Fixed
last Friday at 5pm and (yesterday),19/06/2020 and (tomorrow at 5pm)and20 June 2020 / (in 3 days)now give master's dates, and the bracketed expression is read as a date of its own. The same holds for[...]and"...".- The 82,176 generated sentences of my last review, master against the head: no date that master finds is lost any more (it was 344), and nothing raises.
- A cross-locale sweep (205 locales, about 78,000 sentences with an explicit date next to a bracketed or quoted phrase, weekday or time of that locale): no losses, and no exceptions on the head (master raises in 190 of them).
- Full suite: 24858 passed, 17 skipped, 1 xfailed. The README now says why
wed,satandsunare excluded.
Blocker: a bracketed time without a colon, or with at, is cut off from its date
8 May 2020 (16:05) stays whole now, but (5pm) does not. languages=["en"] and RELATIVE_BASE=datetime(2020, 6, 15, 12), same without languages:
tomorrow (5pm)
master [('tomorrow (5pm', 2020-06-16 17:00)]
head [('tomorrow', 2020-06-16 12:00), ('5pm', 2020-06-15 17:00)]
Friday, June 19 (5pm)
master [('Friday, June 19 (5pm', 2020-06-19 17:00)]
head [('Friday, June 19', 2020-06-19 00:00), ('5pm', 2020-06-15 17:00)]
(5 pm), (at 5pm), (at 17:00), [5pm] and "5pm" do the same, while (17:00), (5:30pm) and (17:00:15) are fine. In a small sweep (11 dates and relative phrases, each followed by a bracketed time), master gets 40 of the 5pm / 5 pm / at 5pm / at 17:00 sentences right and the head loses 36 of them.
Cause: _ENCLOSED_TIME requires a colon, and _attached_group_end does not accept the skip word at, so the group is not treated as part of the date and ends the chunk.
A fix I tested (the suite, the 82k sweep and the cross-locale sweep give the same results with it, and the two tests below fail without it):
_ENCLOSED_TIME = re.compile(r"\d{1,2}(?::\d{2}(?::\d{2})?)?(?:am|pm)?")
...
or (bare in dictionary and dictionary[bare] in _ENCLOSED_WORDS)
or (bare in dictionary and not dictionary[bare]) # skip words such as "at"and two rows for test_search_dates_enclosed_part_of_date:
param(
"en",
"May 5, 2020 (5pm)",
("May 5, 2020 (5pm", datetime.datetime(2020, 5, 5, 17)),
),
param(
"en",
"May 5, 2020 (at 5 pm)",
("May 5, 2020 (at 5 pm", datetime.datetime(2020, 5, 5, 17)),
),Not blocking
- A bracketed time before the date still splits:
(17:00) tomorrowgives17:00today andtomorrowat noon, where master gave tomorrow 17:00. That is an unusual way to write it, so I would leave it. Thu (12 Feb 2016)now returns an extraThu(2020-06-11) next to the right date; master glued the two together.
Happy to look again once this is fixed.
serhii73
left a comment
There was a problem hiding this comment.
Re-review at a4175ad. My earlier requested changes are resolved, so I'm approving.
What I checked on this head:
- The three search_dates regression families from my last review are gone. Every quoted example (modifier next to a date or time, two modified phrases joined by "and", the leaked-RELATIVE_BASE case without a base, bracketed phrases) now returns separate, correct dates. In my sweep of 8820 "A B" sentences with no
RELATIVE_BASE, the first date matchedparse(A)7146 times on this head against 2747 on master, with no case that was right on master and wrong here. - The language-detection blocker is still fixed:
11 Mai 2014,Thu 12 February 2016 11:41,il 5 maggio 2026and11 maigive the same results as master. - Full suite on this head merged with current master: 24960 passed, 17 skipped, 4 xfailed.
- The PR body now resolves only #573, #635 and #1177, so it no longer closes #898 or #1031.
Non-blocking follow-ups:
search_dates('last friday of March')now returns None; master returnedfriday of Marchwith the day wrongly taken from today's date.parse()returns None for it on both. Dropping the stray modifier (or keeping the old result) would avoid losing the match.- A few odd phrasings return None where master gave a wrong date, e.g.
Friday before in 3 daysandMarch 2021 after in 3 days. _clean_substringstrips a trailing)but leaves the opening one, e.g.10 June 2021 (Thursday._trim_connectorsleaveslast friday andwhen other words sit between the two dates.- The modifier vocabulary (
last/this/next) is defined in three modules (search.py,locale.py,parser.py); a shared constant would keep them in sync. _count_words_present_in_the_dictionarynow builds the dictionary on every language scan; a precheck for English-only modifier entries would avoid the cost.
|
Thank you! |
Resolves #573, resolves #635, part of #898, part of #1171, resolves #1177.