icu4x icon indicating copy to clipboard operation
icu4x copied to clipboard

ArithmeticDate::offset_date() doesn't handle wraparounds for months

Open Manishearth opened this issue 2 years ago • 3 comments

The following code:

https://github.com/unicode-org/icu4x/blob/fcdbad1be0b92752f18ba8c63d36352f2781d5ff/components/calendar/src/calendar_arithmetic.rs#L28-L33

will immediately panic if the month offset is negative and less than the number of current months, since there will be an arithmetic overflow.

What this code needs to do is something similar to the ISO code:

https://github.com/unicode-org/icu4x/blob/fcdbad1be0b92752f18ba8c63d36352f2781d5ff/components/calendar/src/iso.rs#L242

where there's a specialty function that allows one to add or subtract months whilst wrapping years.

Note that instead of the hardcoded number 12 we must use the months_for_every_year() value. Furthermore, that value isn't guaranteed to always be constant so when we start supporting lunar calendars we will need to handle that too. I'm tracking that in https://github.com/unicode-org/icu4x/issues/2066

cc @pandusonu2 do you want to work on this? I'd prioritize the 1.0 stuff you're doing, but this is code you're generally familiar with.

Manishearth avatar Jun 13 '22 23:06 Manishearth

The changes dont seem to be anything major, can keep it open for any interested contributor maybe? Would be a good first issue.

pandusonu2 avatar Jun 14 '22 13:06 pandusonu2

Not major if we're just trying to handle solar calendars, they get trickier for lunar. But yeah, makes sense! You can help mentor too :smile:

Manishearth avatar Jun 14 '22 14:06 Manishearth

@yzhang1994 is interested in this

sffc avatar Jun 16 '22 21:06 sffc