Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How should the overflow option behave with leap days and months in PlainMonthDay.from() ? #1237

Closed
justingrant opened this issue Dec 28, 2020 · 10 comments
Labels
non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE!
Milestone

Comments

@justingrant
Copy link
Collaborator

justingrant commented Dec 28, 2020

PlainMonthDay.from() accepts an overflow: 'constrain' | 'reject' option. How should this option behave when the input is a leap day or a day in a leap month? In the ISO calendar this is easy because there's only one leap day and it's always at the end of the same month. So the ISO calendar can use a fixed reference year, which currently is 1972 which was the first leap year after UNIX epoch.

However, non-ISO calendars:

  • Can have leap months (e.g. Hebrew, Chinese) which are not present every year
  • Can have variable-length months, most commonly because month start and endpoints are determined by the position of the moon at a particular place and/or measured in a particular way (e.g. Islamic, Chinese)

This means that the ISO reference year cannot be a constant, and it makes it harder to know how overflow should behave.

My assumption is that the correct overflow behavior in any calendar is this:

  • 'constrain' - try to find a year where this month/day exists. If one can be found, use it for computation of referenceISOYear. If one cannot be found:
    • If month is smaller than any month in any known year, then use the first month in years.
    • If month is larger than any month in any known year, then use the last month in the longest known year
    • If day is smaller than any valid day, then use the first day of the month
    • If day is larger than any day of the month calculated above, then use the largest day of this month in any known year.
  • 'reject' - try to find a year where this month/day exists. If one can be found, use it for computation of referenceISOYear. If one cannot be found, throw a RangeError.

In my non-ISO calendar PR, I'm implementing the logic above by starting from the current date and working backwards one calendar year at a time for up to 100 years to try to find a year where the month/day exists. AFAICT, all built-in calendars have cycle lengths less than 100 years so if a match is going to happen, it should happen within a century. (I'm actually optimizing a little by checking for definitely-invalid values like day: 0 or month: 14 before doing the up-to-100-year loop on maybe-valid month/day pairs, but that optimization shouldn't affect the spec above.)

If you disagree with this approach, LMK soon!

EDIT: How should the calendar react to an invalid leap-month monthCode, e.g. '4L' for any calendar other than Chinese/Dangi. Should it constrain down to the last day of monthCode: '4' per the constrain algorithm in the OP? Or throw a RangeError because that month code is never valid for that calendar?

@Louis-Aime
Copy link

Louis-Aime commented Dec 31, 2020

I have to confess that I do not understand the reference to an ISO year for defining a PlainMonthDay record in any non-ISO calendar, be it solar or not. Despite my non-understanding, I succeeded managing PlainMonthDay in several non-ISO custom calendar, namely: milesian, julian, german (see https://louis-aime.github.io/Temporal-experiences/ ).

IMHO the MonthDay concept is closely related to the corresponding calendar, like the concept of month. The (sole ?) use case of this concept is: given a Year (in the custom calendar) and a MonthDay record, what is the "best fit" date corresponding to (Year, MonthDay) ? This "best fit" date should be computed in the custom calendar (the "target calendar"). Then it is easy to translate this date into any other calendar, including ISO. Hence I do not understand why there should be an ISO reference year in the monthDayFromFields definition.

I understand that another use case could be: which is the date of Hanoucca, the hebrew feast that falls on 25 of Kislev, in a given Milesian year? Since the Milesian calendar has a very tight relation to the ISO, and since 25 Kislev exists in all (hebrew) year, you would say that the solution is straightforward. Not at all. In Milesian 2019, there was no Hanoucca, whereas in 2020 there were two occurrences : 2 1m 2020 (23 Dec. 2019), and 21 12m 2020 (11 Dec. 2020). If you want to avoid citing the Milesian calendar, take the 20 Tevet: no occurrence in ISO 2019, two occurrences in ISO 2020. So I think this use case should be asked in another way.

Ideally, each calendar should define the way of handling the constrain value. However, I think it is reasonable to assume the following general rules, when applying toPlainDate.from method on a given PlainMonthDay to a given { year : } bag, all in a same custom calendar.

  • if year, month, day exist in the target calendar, this is the result; else:
    • if overflow == "constrain"
      • check month, if it is a leap month that does not exist in this year, replace within corresponding common month;
      • check day (in changed month), if this day is above daysInMonth for the 1 of this month in this year, replace with daysInMonth
      • if this substituted {day, month, year} is a valid date in custom calendar, then return the result, else throw;
    • else, throw.

As you can see, there is no reference to any ISO year in this algorithm. And this solves most current cases, like anniversaries of Gioachino Rossini (born Feb. 29 1792), unique 30 Feb. of the Swedish calendar, dates in Adar II or in leap years of the Chinese calendar.

The case of "non-existing" dates around switching dates to the Gregorian calendars would probably lead to an error, even for the very particular case of the "German" calendar (of most protestant states of Germany) in which daysInMonth for Feb. 1700 is 18, the next be being ISO 1700-03-01. These are "extreme" cases, to be solved by the calendar's author in dateFromFields.

BTW I do not either understand the reference to an isoDay when defining yearMonthFromFields, for the same reasons. The reference day should be the first day of the month of the custom calendar. Most generally 1, but 0 for the Maya calendar.

@Louis-Aime
Copy link

Coming back to this simple "use cases":

  • when does 20 Tevet fall in ISO 2019 ?
    I would just give the first date after 2019-01-01, which is 2020-01-17. The same result as for ISO 2020.

@ptomato
Copy link
Collaborator

ptomato commented Jan 5, 2021

I may not be understanding the issue here, so forgive me if this is an answer to the wrong question 😄 The option is intended for when the given month or day are invalid, in other words this combination never exists in any year, for example { month: 2, day: 31 } in the ISO calendar. The approach that Justin gave in the OP seems correct to me and IIRC agrees with what is already done for the ISO calendar.

The part I don't understand is the questions about the reference year, since that should be considered internal to the calendar.

@justingrant
Copy link
Collaborator Author

The approach that Justin gave in the OP seems correct to me and IIRC agrees with what is already done for the ISO calendar.

If we require calendars to canonicalize the reference year, per our discussion in #1239, then the approach in the OP (start from the current year and work backwards) won't work. Instead, each calendar would have to pick a constant year (like we do with 1972 for the Gregorian calendar) and work backwards from there. And once released, the calendar can never change that constant year or it'd break backwards compatibility.

I answered some of @Louis-Aime's questions over in #1239 (comment).

@ptomato
Copy link
Collaborator

ptomato commented Jan 5, 2021

But the value of the reference year doesn't actually matter for the algorithm in the OP? Or maybe I'm misreading it?

@justingrant
Copy link
Collaborator Author

justingrant commented Jan 5, 2021

BTW, I just realized (duh) that because there's a limited set of possible month/day combinations (Chinese has the most of all ICU calendars and it has less than 800 possible combinations), picking a reference year will always be easy via a statically-defined map of (monthCode, day)=>referenceYear. There's no need to mandate a particular algorithm to define the reference years as long as the mapping never changes after the calendar is released.

If it does change, however, then all existing serialized values will be broken, so this is an important restriction. #1239 would make changes less of a big deal, though.

@justingrant
Copy link
Collaborator Author

One more related issue (I'll update the OP too with this info): how should the calendar react to an invalid monthCode, e.g. '4L' for any calendar other than Chinese/Dangi. Should it constrain down to the last day of monthCode: '4' per the constrain algorithm in the OP? Or throw a RangeError because that month is never valid for that calendar?

@ptomato
Copy link
Collaborator

ptomato commented Jan 15, 2021

Maybe I'm still not understanding the question, but I think the status quo is okay?

As for the question about an invalid month code, should "4L" be treated differently than "FOO" if neither are valid month codes for that calendar? And if so, should "100" also be treated differently than either one? In any case given #1229 it seems that would be out of scope for the Temporal spec text, just an implementation detail in the polyfill.

@ptomato ptomato added the non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! label Jan 15, 2021
@ptomato ptomato added this to the Next milestone Jan 15, 2021
@justingrant
Copy link
Collaborator Author

Yep, this is really about documentation for calendar builders (including built-in calendars) not any impact to Temporal spec or API. FWIW, here's the current implementation in the #1245:

  • Non-lunisolar calendars: monthCode must be a numeric string. '4L' will throw regardless of overflow.
  • Chinese/Dangi calendars: if monthCode ends in L but it's not a real month, then constrain will clamp to the last day of the previous month. 1L/12L/13L always throws because those months never exist in any years.
  • Hebrew - if monthCode is 5L (Adar I) in a non-leap year, then constrain will clamp to the last day of the previous month (30 Av). Any other leap codes will always throw.

The general idea is this:

  • leap days will always resolve to the last valid day of that month in the specified year
  • leap months, if in a year where that month doesn't exist, resolve to the last valid day in the previous month
  • leap month codes that never exist in any year will throw, even for constrain.
  • too-small/too-large month indexes will clamp down to the nearest valid day in the nearest valid month in the same year. No wrapping to other years.

I'm not saying those are the correct principles to follow, only that there should be general principles that are followed unless there are cultural practices that override.

The overall goal is to ensure that I can take a {monthCode, day} from one year and apply it to another year and have reasonable outcomes for overflow: 'constrain'. If I have a birthday on a leap day or in a leap month, I still want a cake every year!

@ptomato
Copy link
Collaborator

ptomato commented Jan 19, 2021

Consensus from 2021-01-19 meeting:

  • The calendar itself should be in charge of resolving overflow: 'constrain', according to the cultural practices of users of that calendar. (This is the status quo.)

(We didn't pass any judgement on whether the above principles were correct to be used in the absence of cultural practices, but it seems reasonable to me.)

Closing this issue.

@ptomato ptomato closed this as completed Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE!
Projects
None yet
Development

No branches or pull requests

3 participants