Datetime: narrower internal error types #4864
Conversation
robertbastian
force-pushed
the
errors
branch
10 times, most recently
from
95eb927 to
298c9d2
Compare
There was a problem hiding this comment.
This is good; I would prefer more eliding of error conversions or not introducing all these different private error types, but it's all private and this achieves the stated goal of having a narrower error type on TryWriteable. Although it's not as narrow as I had hoped :)
| .map_err(|e| match e { | ||
| PatternForLengthError::Data(e) => DateTimeError::Data(e), | ||
| PatternForLengthError::Pattern(e) => DateTimeError::Pattern(e), | ||
| })?; |
There was a problem hiding this comment.
Suggestion: you have a lot of this remapping code; it would be a lot less verbose, and I think not harmful, to just implement a From impl and use ? operators.
There was a problem hiding this comment.
I'm explicitly not doing that, because I will introduce more error types in the next PR and then it will become messy.
There was a problem hiding this comment.
Also when doing this I had to recurse a lot to figure out which variants are actually feasible, I don't want the next person to have to do that as well.
There was a problem hiding this comment.
It's hard for me to envision how ? operators make things more messy
There was a problem hiding this comment.
It's unclear at the call site what happens, you don't see what the error type from the nested call is, and which error variants it has.
There was a problem hiding this comment.
We can clean this up when we make the public-facing error changes. This code will be changed more in the short term so I'd like to keep things explicit for now.
There was a problem hiding this comment.
The fact that the internal functions have narrow error types I think is more than enough explicitness. This try_write_field function is extremely complex, perhaps the most complex function in the whole library, and I think the readability of using ? is more important here. I don't care at each and every call site what the different error variants can be, as long as they map cleanly into DateTimeWriteError, which the ? operator guarantees.
robertbastian
requested review from
zbraniecki,
nordzilla,
Manishearth and
a team
as code owners
robertbastian
requested review from
sffc
and removed request for
a team,
zbraniecki,
Manishearth and
nordzilla
| // We don't have any calendars with < 14 days per year, and it's unlikely we'll add one | ||
| debug_assert!(day_of_year_info.day_of_year >= icu_calendar::week::MIN_UNIT_DAYS); | ||
| debug_assert!(day_of_year_info.days_in_prev_year >= icu_calendar::week::MIN_UNIT_DAYS); |
There was a problem hiding this comment.
Are these assertions new, or did they come from somewhere else?
There was a problem hiding this comment.
They protect this result case, and yes, it should have been days_in_year:
icu4x/components/calendar/src/week_of.rs
Line 241 in 3a64209
Based on #4873