Merge ParseError with Error, convert Parsed_set*

[pitdicker]

It is tricky to slice up the work to convert our parsing module from returning ParseError to Error.
After a couple of attempts the best way seems to be to merge the ParseError and Error types first, go over all the methods to clean them up for the new type and remove the constants next, and attach more information such as the location of an error last.

The first two commits merge ParseError and Error, and the last one goes over the Parsed_set* methods.

I have a branch to go over plenty more methods and tests, but this seems like enough to review in one PR.

[codecov]

Codecov Report

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

Project coverage is 94.05%. Comparing base (21ee9b7) to head (983d846).

❗ Current head 983d846 differs from pull request most recent head 36def24. Consider uploading reports for the commit 36def24 to get more accurate results

Files	Patch %	Lines
src/error.rs	50.00%	3 Missing ⚠️
src/format/parse.rs	83.33%	2 Missing ⚠️
src/format/parsed.rs	98.41%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            0.5.x    #1511      +/-   ##
==========================================
+ Coverage   93.96%   94.05%   +0.09%     
==========================================
  Files          37       37              
  Lines       16662    16972     +310     
==========================================
+ Hits        15656    15963     +307     
- Misses       1006     1009       +3     

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

[pitdicker]

As an extra note: the intention of this commit by merging ParseError into Error is to make intermediate states possible.

With this we can work on methods that currently return ParseError one by one without needing commits of many 100 lines and weird workarounds.

The addition of the Ambiguous and Inconsistent variants are the only parts I do not intent to change later.

[djc]

Both of these error variants seem pretty specific to parsing, so I wonder if there's a different way of modeling these that would avoid having this in the top-level Error variant (since they couldn't occur in most of the places where Error happens).

Like, maybe ParseError gains an Invalid(Error) variant?

[pitdicker]

Ambiguous was mostly planned as an error variant that corresponds to MappedLocalTime::Ambiguous, similar to how DoesNotExist is planned to correspond to the current MappedLocalTime::None. It is nice that Ambiguous also makes sense for what is currently ParseError::NotEnough.

Inconsistent does not feel entirely unreasonable to have. I must admit its only use is with the Parsed type currently, which is of course closely related to parsing.

Errors during parsing have an overlap with our general errors, only InvalidArgument would not be returned by it.
Personally I see no problem in including three or four parsing-specific variants in our Error enum. I very much like to see them all in a single type with a single level (not nested) because that seems like the most convenient for our users.

As I currently see it there is going to be one more variant in the Error enum after this to wrap the error type from #1448 (comment).

(since they couldn't occur in most of the places where Error happens).

Errors related to time zone conversion don't happen with the naive types, and only OutOfRange applies to TimeDelta and FixedOffset. Still it seems helpful to have them return the same type.

[djc]

(since they couldn't occur in most of the places where Error happens).

Errors related to time zone conversion don't happen with the naive types, and only OutOfRange applies to TimeDelta and FixedOffset. Still it seems helpful to have them return the same type.

My point here is that there's a balance to strike here, and also there's some asymmetry here:

• If, say, 40% of the top-level Error variants can only be triggered by 10% of functions returning it, that doesn't seem great -- I'm not sure if the parsing-specific variants for chrono's Error would be like that, but that would be my impression?
• On the other hand, it's fine to have a small number of functions that can only trigger a small percentage of the Error variants (because the alternative of introducing a large number of error types is worse)

[pitdicker]

What advantages do separate type for parse errors bring?

• The extra errors that can arise when parsing don't pollute the regular enum with three or four extra variants.
• Users that never use the parsing functionality of chrono don't have to write a conversion method to their own error type for parsing error variants.

What advantages does it bring to have one error type?

• We consistently return one error type.

All in all the advantages of either one or two errors are very tiny and theoretical. The types can have the same size (if we care, I do). Users can rarely if ever exhaustively match on the Error variants of any method, I'm not sure in the end we will have even one method than can return all variants. And we don't mark the enum as [non_exhaustive] for nothing, users are not expected to do exhaustive matching.

So it comes down to preference. Then my preference is for simplicity: one error type.

[djc]

Okay: let's do single error type for now, but I think we should reconsider splitting it before the 0.5.0 release.