Parsing preparations

[pitdicker]

These issues came up when trying to convert our parsing code to the new error types (only 750 errors to go... 😆).

I would like to restrict the visibility of ParseError to the format module on 0.5, and make all parsing go through format::parsed.
That means the FromStr implementations of DateTime<FixedOffset> and FixedOffset should use the standard parsing methods instead of reaching directly for the functions in format::{parse, scan}.

In timezone_offset I simplified digit parsing a bit because it did not have quite the places to hook up new error values as I wanted. It remains a pretty broken parser though.

Further I noticed a 0i64.checked_sub(v).ok_or(OUT_OF_RANGE) line. Subtracting a positive i64 from 0 can never fail. Then I noticed we don't support parsing negative timestamps at all! That is fixed in the last commit. Parsing i64::MIN was a little tricky.

[codecov]

Codecov Report

Attention: Patch coverage is 98.07692% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 91.83%. Comparing base (391187f) to head (3b7fa22).

Files	Patch %	Lines
src/format/scan.rs	96.77%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1479   +/-   ##
=======================================
  Coverage   91.82%   91.83%           
=======================================
  Files          37       37           
  Lines       18173    18166    -7     
=======================================
- Hits        16687    16682    -5     
+ Misses       1486     1484    -2     

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

[djc]

What are we trying to do here? Why do we need to support negative timestamps? Having to pass in true for positive all over the place doesn't look great.

[pitdicker]

Why do we need to support negative timestamps?

We support negative timestamps everywhere, it seems like an oversight to not support them with the %s formatting specifier when parsing.

Having to pass in true for positive all over the place doesn't look great.

My problem was that I wanted to do a complete fix instead of partial. I.e. to also support parsing i64::MIN (which can be stored in Parsed even though it is out of range for NaiveDateTime, and is something I expect users may test against). Because -i64::MIN can't be represented in an i64 this function should either return an u64, or it must return a value with the correct sign. Returning with the correct sign was the minimal change.

[djc]

Well, I think it would be better to turn number() into a helper wrapper of an underlying parsing function.

[pitdicker]

👍 I'll do that.

[pitdicker]

Added another commit to remove some duplicated length checks. They where just before scan::nanosecond_fixed, which uses scan::number that does the same checks.

[djc]

I'm very confused by the logic of parsing a non-negative number by first parsing a negative number and then negating it...

[pitdicker]

I thought it was quite nice. Positive numbers are not 'better' than negative numbers, and in this case negative numbers had the one integer larger range we needed.

Updated to parse to an u64 and convert with a tricky cast instead.

[djc]

Can you explain in the commit message why they're duplicate (that is, how these invariants are upheld)? It's not clear in the diff.

[djc]

I'd prefer to inline this in the caller (more like before), since there aren't any other users, anyway.

Is there a way to more directly express the precondition for negating the number? This feels kind of roundabout -- even the previous 0i64.checked_sub(v).ok_or() feels clearer.