Fix up eras, correctly expose arithmetic year#424
Conversation
Manishearth
force-pushed
the
extended-year
branch
from
65a70cf to
8b38b69
Compare
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
Looks good to me overall. I did have one question regarding an error message, but it's not necessarily blocking.
| } | ||
| AnyCalendarKind::Japanese if *era_alias == tinystr!(19, "mejei") => { | ||
| Some(era::MEJEI_ERA) | ||
| AnyCalendarKind::Japanese if *era_alias == tinystr!(19, "meiji") => { |
| let calculated_arith = era_info.arithmetic_year_for(era_year); | ||
| if calculated_arith != arith { | ||
| return Err( | ||
| TemporalError::range().with_message("Conflicting era/eraYear info") |
There was a problem hiding this comment.
question: shouldn't this be for conflicting year and eraYear values?
There was a problem hiding this comment.
ah, yes, correct. will fix when I'm back at a computer
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
oops my comments never got posted
| (Some(year), None, None) => Ok(Self { | ||
| era: partial | ||
| .calendar | ||
| .get_calendar_default_era() |
There was a problem hiding this comment.
in theory here we should be able to just pass down None; ICU4X treats None as "default era"
| }) | ||
| } | ||
| (None, Some(era), Some(era_year)) => { | ||
| (maybe_year, Some(era), Some(era_year)) => { |
There was a problem hiding this comment.
This does not yet handle cases like (year, era, eraYear = none) or (year, era = None, eraYear). They're a bit more nonsensical; I'm not sure if we should be doing anything here.
| // or a RangeError exception if the fields are sufficient but their values are internally inconsistent | ||
| // within the calendar (e.g., when fields such as [[Month]] and [[MonthCode]] have conflicting non-unset values). For example: | ||
| if let Some(arith) = maybe_year { | ||
| let calculated_arith = era_info.arithmetic_year_for(era_year); |
There was a problem hiding this comment.
note that this deliberately compares arithmetic years instead of trying for the default era: (2019, reiwa, 1) is an okay thing to specify.
| let calculated_arith = era_info.arithmetic_year_for(era_year); | ||
| if calculated_arith != arith { | ||
| return Err( | ||
| TemporalError::range().with_message("Conflicting era/eraYear info") |
There was a problem hiding this comment.
ah, yes, correct. will fix when I'm back at a computer
2 participants
This does a bunch of fixes to the era code:
.year()as arithmetic yearFixes #423