Clean slate with Winnow 2 #86

dhilst · 2024-08-29T22:09:25Z

Continuing the rewrite started by @tertsdiepraam #61

The functions in lib.rs keep the same signature
I kept the tests in lib.rs too all they are passing
90% of the parsing was written by @tertsdiepraam (check Clean slate with Winnow #61)
Added timestamp with @\d+ format
Added timezone abbreviation names
Added 8 digits dates MMDDhhmm
Using winnow

dhilst · 2024-08-29T22:09:55Z

@tertsdiepraam I squashed your commits into a single commit, I hope you don't mind

tertsdiepraam · 2024-08-30T08:25:24Z

Looks cool! A few initial comments to send you in the right direction. One of the reasons for this rewrite is that chrono wasn't cutting it for adding all the items together. Instead we should add all the items together ourselves to really match GNU. It'll require a lot of experimentation, but it'll make the compatibility much better. I think most of the corner cases were around the ends of months. I believe GNU had some overflow behaviour there that chrono didn't. So I recommend first creating a custom datetime struct to manipulate and then mapping that to chrono.

Squashing the commit is absolutely fine by the way. Thanks for continuing this!

codecov · 2024-08-31T05:01:28Z

Codecov Report

Attention: Patch coverage is 65.92449% with 352 lines in your changes missing coverage. Please review.

Project coverage is 62.66%. Comparing base (3813a3b) to head (374e49e).

Files with missing lines	Patch %	Lines
src/items/time.rs	43.69%	13 Missing and 246 partials ⚠️
src/lib.rs	63.41%	16 Missing and 14 partials ⚠️
src/items/mod.rs	87.77%	3 Missing and 19 partials ⚠️
src/items/date.rs	85.05%	0 Missing and 13 partials ⚠️
src/items/weekday.rs	75.00%	1 Missing and 12 partials ⚠️
src/items/ordinal.rs	61.29%	0 Missing and 12 partials ⚠️
src/items/relative.rs	98.14%	1 Missing and 1 partial ⚠️
src/items/combined.rs	96.96%	0 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (3813a3b) and HEAD (374e49e). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (3813a3b) HEAD (374e49e)

macos_latest 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #86       +/-   ##
===========================================
- Coverage   85.93%   62.66%   -23.28%     
===========================================
  Files           6        9        +3     
  Lines         853     1248      +395     
  Branches      188      409      +221     
===========================================
+ Hits          733      782       +49     
- Misses        117      134       +17     
- Partials        3      332      +329

Flag	Coverage Δ
macos_latest	`?`
ubuntu_latest	`62.60% <65.92%> (-6.26%)`	⬇️
windows_latest	`19.93% <23.61%> (+15.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

dhilst · 2024-09-14T02:46:57Z

@tertsdiepraam I was looking into the fuzzy testing. I added the code so it generates fuzzy dates with some sanity and caught some problems, I will look into them next.

dhilst · 2024-09-14T03:38:49Z

fuzz/fuzz_targets/parse_datetime.rs

+    // Testing GNU compatibility by calling date and
+    // comparing the inputs
+
+    // let fmt = "%Y-%m-%d %H:%M:%S";
+    // let mut output: process::Output = std::process::Command::new("date")
+    //     .arg("-d")
+    //     .arg(s)
+    //     .arg(format!("+{fmt}"))
+    //     .output()
+    //     .expect("date command failed");
+    // io::stdout().write_all(&output.stdout).unwrap();
+    // io::stdout().write_all(&output.stderr).unwrap();
+    // output.stdout.pop(); // remove trailing \n
+    // assert_eq!(
+    //     String::from_utf8(output.stdout).unwrap(),
+    //     d.format(fmt).to_string()
+    // );


I was testing something like this to determine its compatibility with GNU. The test run GNU date in and compare the outputs

We could definitely do that with fuzzing! We should still rely on regular tests as well though.

dhilst · 2024-09-14T21:19:10Z

src/lib.rs

+        #[test]
+        fn chrono_date() {
+            const FMT: &str = "%Y-%m-%d %H:%M:%S";
+            let input = "262144-03-10 00:00:00";
+
+            // chrono rejects this specific date
+            assert!(NaiveDate::from_ymd_opt(262144, 3, 10).is_none());
+            assert!(chrono::DateTime::parse_from_str(input, FMT).is_err());
+            // the parsing works, but hydration fails
+            assert!(items::parse(&mut input.to_string().as_str()).is_ok());
+            assert!(parse_datetime(input).is_err());
+            // GNU date works
+            assert_eq!(make_gnu_date(input, FMT), input);
+        }


@tertsdiepraam I found a case where GNU accepts the date, we can parse it and chrono rejects it, more precisely chrono::NaiveDate::from_ymd_opt returns None for this date. I don't know what direction to take from there, chrono keeps getting in the way 😅 ideas? Maybe having a gnu-compatible datetime layer, but that will be a lot of work

Another problem is with formatting too. chrono puts a + in front of the year if the year is >9999. This breaks some tests:

sylvestre · 2024-09-17T07:39:39Z

sorry but some tests are failing:

failures:
    items::date::tests::test_year
    items::date::tests::with_year
    items::tests::date_and_time
    tests::offsets::invalid_offset_format
    tests::test_relative::chrono_date
    tests::test_relative::gnu_compat
    tests::test_relative::test_month

and needs some rebasing

dhilst · 2024-09-17T10:55:16Z

sorry but some tests are failing:

failures:
    items::date::tests::test_year
    items::date::tests::with_year
    items::tests::date_and_time
    tests::offsets::invalid_offset_format
    tests::test_relative::chrono_date
    tests::test_relative::gnu_compat
    tests::test_relative::test_month

and needs some rebasing

I will rebase it

But right now I have a problem with Chrono, I'm looking into alternatives like https://time-rs.github.io/book/how-to/create-dates.html

dhilst · 2024-09-25T21:39:34Z

Hello, I updated the PR, some key notes:

I found that if we use only years in the range 0 to 9999, Chrono and GNU behavior coincide. I update the PR with such a fix, the fuzz tests for example are generating years in this range.
GNU accepts years in the range 0 to 2147485547
Chrono accepts years in the range 0 to 262144
But when using years >9999 the chrono add a '+' the output format where GNU doesn't

Let me know what you think, if this is too bad, or if it is okay for now. I guess that to achieve 100%
GNU compatibility we'll need to do our own date math.

BTW I was playing with GNU date and I found this behavior

TZ=UTC date -d '0000-01-01 00:00 utc+00001' '+%Y'
-001

Here -001 is the year, it looks like it is subtracting 1 second from the date (which is 0)
and it underflows to -1, I guess this is a bug, here is again all the output

Z=UTC date -d '0000-01-01 00:00 utc+00001' 
Fri Dec 31 11:59:00 PM UTC -001

sylvestre · 2024-09-26T06:12:36Z

Cargo.toml

+[features]
+debug = ["winnow/debug"]
+
+[[bin]]


should be done in a different PR, no?

Yes! I added to compare some dates with GNU, I'm removing this

sylvestre · 2024-09-26T06:12:53Z

fuzz/Cargo.toml

@@ -15,6 +15,9 @@ chrono = { version="0.4", default-features=false, features=["std", "alloc", "clo
 [dependencies.parse_datetime]
 path = "../"

+[features]


same, why in this PR ?

To see the trace output I need build it with winnow/debug/ feature. I tried cargo fuzz --features winnow/debug but it didn't worked. So I added this debug feature that depends on winnow/debug and I can enable it instead

dhilst mentioned this pull request Aug 29, 2024

[WIP] Add date format fallbacks uutils/coreutils#6667

Closed

dhilst force-pushed the clean-slate-2 branch 10 times, most recently from ff67b9e to 85d7574 Compare August 30, 2024 02:02

dhilst force-pushed the clean-slate-2 branch 16 times, most recently from 5516eed to 9388e45 Compare August 30, 2024 20:52

dhilst commented Sep 14, 2024

View reviewed changes

dhilst force-pushed the clean-slate-2 branch 5 times, most recently from 2389d91 to 025808c Compare September 14, 2024 20:59

dhilst commented Sep 14, 2024

View reviewed changes

dhilst force-pushed the clean-slate-2 branch 5 times, most recently from 307c159 to a434820 Compare September 15, 2024 00:58

dhilst force-pushed the clean-slate-2 branch 3 times, most recently from fb36b99 to c5b5b9a Compare September 25, 2024 21:11

dhilst force-pushed the clean-slate-2 branch 2 times, most recently from 9eefad4 to 7a43f13 Compare September 25, 2024 21:51

sylvestre reviewed Sep 26, 2024

View reviewed changes

dhilst force-pushed the clean-slate-2 branch 3 times, most recently from 543d529 to 235086b Compare September 27, 2024 20:15

tertsdiepraam and others added 3 commits October 1, 2024 10:19

start parsing date with winnow

98dc6aa

Restore the old API and add the missing combinators

98599f1

Find a compatibility intersection between chrono and GNU

374e49e

dhilst force-pushed the clean-slate-2 branch from 235086b to 374e49e Compare October 1, 2024 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean slate with Winnow 2 #86

Clean slate with Winnow 2 #86

dhilst commented Aug 29, 2024 •

edited

Loading

dhilst commented Aug 29, 2024

tertsdiepraam commented Aug 30, 2024

codecov bot commented Aug 31, 2024 •

edited

Loading

dhilst commented Sep 14, 2024

dhilst Sep 14, 2024

tertsdiepraam Sep 14, 2024

dhilst Sep 14, 2024

dhilst Sep 15, 2024

sylvestre commented Sep 17, 2024

dhilst commented Sep 17, 2024

dhilst commented Sep 25, 2024

sylvestre Sep 26, 2024

dhilst Sep 26, 2024

sylvestre Sep 26, 2024

dhilst Sep 26, 2024 •

edited

Loading

Clean slate with Winnow 2 #86

Are you sure you want to change the base?

Clean slate with Winnow 2 #86

Conversation

dhilst commented Aug 29, 2024 • edited Loading

dhilst commented Aug 29, 2024

tertsdiepraam commented Aug 30, 2024

codecov bot commented Aug 31, 2024 • edited Loading

Codecov Report

dhilst commented Sep 14, 2024

dhilst Sep 14, 2024

Choose a reason for hiding this comment

tertsdiepraam Sep 14, 2024

Choose a reason for hiding this comment

dhilst Sep 14, 2024

Choose a reason for hiding this comment

dhilst Sep 15, 2024

Choose a reason for hiding this comment

sylvestre commented Sep 17, 2024

dhilst commented Sep 17, 2024

dhilst commented Sep 25, 2024

sylvestre Sep 26, 2024

Choose a reason for hiding this comment

dhilst Sep 26, 2024

Choose a reason for hiding this comment

sylvestre Sep 26, 2024

Choose a reason for hiding this comment

dhilst Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

dhilst commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 31, 2024 •

edited

Loading

dhilst Sep 26, 2024 •

edited

Loading