Implement the Japanese calendar #1394
Conversation
Manishearth
requested review from
gregtatum,
nordzilla,
sffc,
zbraniecki and
a team
as code owners
Manishearth
force-pushed
the
japanese
branch
from
e636b3d
to
859365a
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
| day: 1, | ||
| }; | ||
|
|
||
| const FALLBACK_ERA: (EraStartDate, TinyStr16) = (REIWA_START, tinystr16!("reiwa")); |
There was a problem hiding this comment.
question: is there a good reason for that to be hardcoded? Should it be in some data file?
There was a problem hiding this comment.
The era start dates are never going to change, this is an optimization
There was a problem hiding this comment.
And the fallback era will only be hit if the data file has bugs.
| let era_data = self.eras.get(); | ||
| let deref_tuple = |x: (&EraStartDate, &TinyStr16)| (*x.0, *x.1); | ||
| if date >= MEIJI_START { | ||
| if era_data.dates_to_eras.len() == 5 { |
There was a problem hiding this comment.
question: how did you chose 5 as a cut-off for hardcoded eras? Can you document your thinking to help someone one day follow the logics when updating it if needed?
There was a problem hiding this comment.
Oh, "Meiji" onwards are the five modern eras.
Manishearth
force-pushed
the
japanese
branch
from
44f2163
to
ea335be
Compare
|
Argh, needed to rebase |
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
| "stand_alone": null | ||
| }, | ||
| "eras": { | ||
| "names": { |
There was a problem hiding this comment.
Issue: including the premodern era names bloats data size. I recall us discussing that premodern eras would use Gregorian eras.
There was a problem hiding this comment.
I don't recall that being resolved, perhaps we should have a followup? I think the resolution was that premodern eras get the more complex era codes.
There was a problem hiding this comment.
I think we may consider shipping the historic eras as an optional extension. But yes, we should file an issue, because this is simply way too much data without a great way to slice it
There was a problem hiding this comment.
Yeah we can have calendar/japanese@1/{modern, historical} variants and by default it only loads modern. An interesting question would be if it should load historical unconditionally if it's present in the data, or if it should be an option during construction (enum Historical { Yes, No, Try })
A nice property of such an architecture is that we can have the hardcoded fallback for when modern data is unavailable as well.
There was a problem hiding this comment.
This feels like a good mentorable help wanted bug
Fixes #1116
This data is not zero-copy (followup at #1393, it is blocked on zbraniecki/tinystr#56), but otherwise this works fine.
I elected to put end-to-end tests for japanese era data as a separate datetime fixture.
This change does not consider era breaks to change the number of "days in a year"; I'm not sure if this really should be the case but a lot of other things get very confusing (how does adding a year work around an era break?).