formalize grammar of relative time strings and use nom to parse them

[yuankunzhang]

BREAKING CHANGES: this commit introduced two breaking changes:

• Keyword ago semantic change (to be consistent with GNU date)

  The ago keyword now applies to individual date-time displacement (the one that immediately precedes it) and can be specified multiple times. For example, "1 day ago 2 days ago" gives a combined effect of "3 days ago", and "1 day 2 days ago" gives a combined effect of "1 day ago". This closes The keyword ago behaves differently here than in GNU date. #131.

  In previous implementation, ago can only be specified once at the end of the relative time string, and it applies to all date-time displacements as a whole. So "1 day ago 2 days ago" is invalid and "1 day 2 days ago" gives a global effect of "3 days ago".

• Relative time strings like "lastweek" or "nextmonth" are now invalid, and space(s) are required in between the shift and the time unit.

[Copilot]

Pull Request Overview

This PR formalizes the grammar of relative time strings by replacing the regex‐based parser with a nom-based implementation, and adjusts the behavior of the “ago” keyword for consistency with GNU date.

• Replaces regex parsing in src/parse_relative_time.rs with nom-based parsing via a new module in src/parse/mod.rs
• Breaks previous global “ago” behavior by applying it only to the immediately preceding displacement and requires spaces between time shift and unit
• Updates tests to reflect the new semantics in relative time calculations

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/parse_relative_time.rs	Refactored parsing logic to use parse::parse_relative_times and updated displacement handling
src/parse/mod.rs	Introduced new module encapsulating relative time parser logic
src/lib.rs	Added module export for parse

Comments suppressed due to low confidence (2)

src/parse_relative_time.rs:58

• Ensure that the new parsing logic encodes the 'ago' semantics in the RelativeTime values. Currently, displacement variants are always processed with a false is_ago flag, so verify that negative adjustments are correctly applied via the parser.

let relative_times = parse::parse_relative_times(s.trim()).map_err(|_| ParseDateTimeError::InvalidInput)?;

src/parse_relative_time.rs:290

• Verify that the change from using is_ago true to false in month adjustments aligns with the intended 'ago' behavior. Confirm that the sign inversion is handled appropriately, possibly within the parser, so that tests and implementation remain consistent.

add_months(now, 1, false)

[sylvestre]

how does this compare with #86 ?

[yuankunzhang]

how does this compare with #86 ?

It is a revamp of #86, I would like to restart the effort to replace the regex-based parser as I saw that the development of #86 was inactive for a while.

[sylvestre]

Thanks :)

[sylvestre]

Did you compare performances?

[yuankunzhang]

Did you compare performances?

I did some preliminary bench and saw some performance improvements. See below results.

Before this PR:

parse_datetime_strings  time:   [729.84 µs 741.39 µs 751.78 µs]
                        change: [+0.3733% +2.1231% +3.7530%] (p = 0.02 < 0.05)
                        Change within noise threshold.

After this PR:

parse_datetime_strings  time:   [230.61 µs 230.82 µs 231.03 µs]
                        change: [-69.522% -69.178% -68.810%] (p = 0.00 < 0.05)
                        Performance has improved.

[Copilot]

Pull Request Overview

This PR formalizes the grammar for relative time strings and updates the parsing logic to treat the "ago" keyword with individual displacement semantics and require explicit spacing between time modifiers. Key changes include:

• Refactoring and renaming of weekday parsing from parse_weekday to parse_weekday_at_date with proper ordinal adjustment.
• Replacing regex‐based relative time parsing with a nom-driven parser in parse_relative_time.rs and updating tests to reflect new "ago" semantics.
• Introducing new parser modules (primitive and weekday) with enhanced grammar definitions and improved error handling.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/parse_weekday.rs	Updated weekday parser with ordinal adjustments and new API.
src/parse_relative_time.rs	Migrated to nom-based relative time parsing and updated tests.
src/parse/weekday.rs	Added a dedicated module for parsing weekday strings.
src/parse/primitive.rs	Introduced improved parsers for ordinal and integer handling.
src/parse/mod.rs	New module aggregating parsing utilities.
src/lib.rs	Updated imports and integration to use the new weekday API.

Comments suppressed due to low confidence (2)

src/parse_weekday.rs:19

• [nitpick] Consider adding an inline comment explaining why the ordinal value is decremented when the current weekday does not match the parsed weekday; this will improve clarity for future maintainers.

let mut ordinal = ordinal.unwrap_or(0);

src/parse_relative_time.rs:290

• Verify that switching the boolean flag from 'true' to 'false' for add_months in this test aligns with the intended new behavior of the 'ago' keyword applying per displacement rather than globally.

add_months(now, 1, false)

[Copilot]

Update the surrounding documentation/comments to reflect the change in API from parse_weekday to parse_weekday_at_date, noting that a reference date is now required for accurate weekday resolution.

Suggested change

	// parse weekday
	// Parse weekday using `parse_weekday_at_date`.
	// Note: `parse_weekday_at_date` requires a reference date (`date`) for accurate weekday resolution.

[tertsdiepraam]

Hi! I'm honestly a bit torn about this PR. It contains some good changes, but it's - as far as I can tell - not quite like #86. The point of #86 was to get closer to the GNU version. I cannot reproduce some of the formats you allow here in GNU (e.g. "today and tomorrow") and GNU also supports many things you don't have here (e.g. "tue.").

This PR is a good effort, but I see too many slight differences with what I expect from a GNU-compatible implementation. So, if this introduces many slight differences with GNU, I'm hesitant to accept this over #86.

A better forward would be to

• either take all the tests from Clean slate with Winnow 2 #86 and add them here, look at Clean slate with Winnow 2 #86 and fix up the differences,
• or if you want you could make a new PR based on Clean slate with Winnow 2 #86 instead.

If the other maintainers want this, we can still merge this as long as #86 is stuck, because it's still a clear improvement.

[yuankunzhang]

Hi @tertsdiepraam , thanks for your feedback! Formats like "today and tomorrow" and "today, tomorrow" has always been supported in the crate and It is good to keep the support for them for compatibility purpose.

I'll see what I can do to make #86 running, and perhaps we can go from there.

One question though, do we want to adapt #86 to use nom or keep using winnow as is?

[tertsdiepraam]

Thank you! I preferred Winnow back then, but nom has been improved in the meantime. Both are fine with me.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (b6795d9) to head (98826c1).
Report is 2 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff     @@
##   main   #132   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.