fallible offset conversion #145
Conversation
yuankunzhang
force-pushed
the
fallible-offset-conversion
branch
from
1799a31 to
e96aedd
Compare
|
please add test to make sure it doesn't happen in the future, thanks! |
yuankunzhang
force-pushed
the
fallible-offset-conversion
branch
from
e96aedd to
ab91b2b
Compare
| #[test] | ||
| fn offset_overflow() { | ||
| assert!(parse_datetime("m+12").is_err()); | ||
| assert!(parse_datetime("24:00").is_err()); |
There was a problem hiding this comment.
In another PR I had the fuzzer fail on "m1y-12".
Maybe worth adding?
assert!(parse_datetime("m1y-12").is_ok());
(-12 is an acceptable offset)
There was a problem hiding this comment.
Maybe I misunderstand something, but shouldn't it be:
assert!(parse_datetime("m1y-12").is_err());and
assert!(parse_datetime("m+12").is_ok());With GNU date I get:
$ date -d "m1y-12"
date: invalid date ‘m1y-12’
$ date -d "m+12"
Sun May 25 02:00:00 AM CEST 2025
There was a problem hiding this comment.
Goodness....
So I have no idea what m1y is supposed to parse to ,-P But it seems like we accept it, while GNU coreutils always rejects it. So I think this is orthogonal to the problem here. So maybe let's drop my suggestion. Filed #147.
BUT, GNU coreutils accepts all offsets between -24 and +24, so we should do that too:
$ date -d "m"
Sun May 25 02:00:00 PM CEST 2025
$ date -d "m+24"
Sat May 24 02:00:00 PM CEST 2025
$ date -d "m+25"
date: invalid date ‘m+25’
$ date -d "m-25"
date: invalid date ‘m-25’
$ date -d "m-24"
Mon May 26 02:00:00 PM CEST 2025
There was a problem hiding this comment.
We are using chrono::FixedOffset to represent offsets, which accepts offsets only in the range -24 to +24. Note that the literal m+12 will overflow because the letter m already denotes a +12 offset. Achieving GNU date compatibility in this regard will require re-working the underlining data models. Since this PR is just a trivial fix, I suggest we merge it first and address the larger redesign in a separate effort.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #145 +/- ##
===========================
===========================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.