-
Notifications
You must be signed in to change notification settings - Fork 157
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
Problem with Temporal.Calendar.monthDayFromFields #1636
Comments
My expectation would have been B and E. The fields should be constrained to |
Why 1972? |
A. I believe the current spec make it B and D (not B and E) |
I would expect B and D but that's maybe obvious since I reviewed this part of the spec and am quite familiar with it (I think Cam wrote it.) Before going into details I'm curious if my intuition matches @justingrant's, who was the designer of this logic.
1972 is an arbitrarily chosen reference year for PlainMonthDays in the ISO calendar - it is the first leap year after the Unix epoch. (See step 11 of ISOMonthDayFromFields and #391 for even more background.) |
Could someone write a simple explaination of why it is B and D but not B and E to "a user who will not look at the spec text" and communicate of the rational via "plain English" why out2 is D but not E? but out1 is B? I understand the spec make it D but not E but I have hard time to understand the logic to design the spec in such a way. I believe normal user will also very confused. What do we want to achieve by such differences? (one return 28 w/o monthCode: "M02" and the other return 29 because it has monthCode: "M02") |
This is indeed complicated. Here's my general understanding:
Frank brings up interesting cases:
I think the answer is to constrain the month and day to that year, and then use the resulting ISO date to calculate the The wildcard here is whether the ISO calendar should behave differently from non-ISO calendars which should all use behavior (B). I know in
I think a reasonable way to handle this case would be to ignore There are more cases to consider:
I think the easiest rule to handle all these cases is that Another option would be to require that redundant data always matches, and we should throw in case of conflicts. How is the spec currently written? Note that none of these solutions are perfect. I'm open to being convinced of a different position. |
ok, If I follow this logic, then {year: 2021, month: 2, day: 29}, The later operation of using 1972 as reference could then be used with {month: 2, day: 29, monthCode: "M02"}
right? In other words, we need to define what the isoYear here mean. Is it the year we used to constrain the MonthDay? if it is, then we should either use 2021 to constrain the day as 28 and put 2021 into isoYear (out1=C), or use 1972 to constain the day as 29 and report isoYear as 1972 (out1=A) but I see no reason out1=B. |
@justingrant If What happens with
@FrankYFTang I think your suggestion (output equals A) is a self-consistent alternative, and I would be fine with adopting it. But I wouldn't say there is no reason for the output to equal B. The mental model of B is:
(Maybe your mental model of B is missing the second step? The issue explaining it is #1239 and the consensus from the champions is described in #1239 (comment) plus a clarification from Justin in #1239 (comment))) In other words B ensures these operations are the same: (note that I personally don't care that much about this consistency, but maybe other champions do!) input = {year: 2021, month: 2, day: 29}
out1 = Temporal.PlainMonthDay.from(input).getISOFields()
// out1 is { calendar, isoDay: 28, isoMonth: 2, isoYear: 1972 }
out2 = Temporal.PlainMonthDay.from(Temporal.PlainDate.from(input)).getISOFields()
// out2 is the same as out1 Also note that |
I think the interesting part is the 1972. If we decide to use 1972 eventually, regardless what the user give us, then why not use 1972 FIRST while deciding the monthCode by ignoring 2021 in that step too since eventually we are going to ignore 2021 and use 1972 anyway, right? I am NOT feeling strongly with this, but it is odd to me that if we want to use 1972 eventually and ignore 2021 as year, then why not igore 2021 in the very beginning but only igore it in the end. Also, since we are going to use 1972 anyway, shouldn't we just use 1972 as year while it is not present instead of throw ? |
ok, at this point I ask too many questions about this and I think it is detail which any answer have some merit and any answer will always have some strange thing. So... not to waste your time, let me just close it. It is not a big deal as long as you folks confirm this is by design but not a mistake. (I cannot tell until I ask since there are mistakes I found in some other places.) |
Yep, this behavior is by design. If the user gives us a YMD input, we expect it to be a valid date and will constrain or reject it just like we would for any other date construction. In theory we could use YMD to calculate the Note that the reference year algorithm is calendar-specific and is not defined in 262 spec except for ISO. The only requirement on the calendar implementations is that for any |
@justingrant any reason not to omit the year entirely? |
@ljharb do you mean why does PlainMonthDay care about the reference year at all? Or how can the user avoid providing the year when calling |
I mean like, why make "1972" an observable value anywhere |
Without it being observable, PlainMonthDay in a non-ISO calendar wouldn't be serializable as an ISO string. |
if there's no ISO string format for just "month and day", why would one need an ISO string for a MonthDay without also explicitly providing a year to go along with it? |
There needs to be some string serialization format for all Temporal types, e.g. so you can store them in a single database column and later deserialize back into an ECMAScript instance. The convention across Temporal is that all Temporal types are string-serializable using ISO grammar, including the timezone and calendar extensions that @ryzokuken is working with IETF on standardizing. We could invent a new string serialization format that used calendar units (not ISO) which I think would remove the need for an observable ISO year, but that would be different behavior from all other Temporal types. Hard to see how that divergence would be better. Or am I misunderstanding what you're asking? |
I'm wondering if perhaps that MonthDay serialization require an explicit year, like |
One challenge with that approach (in addition to the worse serialization ergonomics in all calendars, including ISO where the year is irrelevant) is that PlainMonthDay instances that are serialized by different developers could have different string serializations. If you're communicating with an external store or service, then it's really helpful to have a deterministic serialization format, where "deterministic" means that if Keep in mind that the year only shows up when serializing PlainMonthDay for non-ISO calendars. For the ISO calendar, the format is Temporal.PlainMonthDay.from({ month: 1, day: 1 }).toString();
// => "01-01"
Temporal.PlainMonthDay.from({ monthCode: 'M01', day: 1 }).toString();
// => "01-01"
Temporal.PlainMonthDay.from({ monthCode: 'M01', day: 1, calendar: 'iso8601' }).toString();
// => "01-01"
Temporal.PlainMonthDay.from({ monthCode: 'M01', day: 1, calendar: 'chinese' }).toString();
// => "1971-01-27[u-ca=chinese]"
Temporal.PlainYearMonth.from({ year: 2020, month: 1, calendar: 'chinese' }).toString();
// => "2020-01-25[u-ca=chinese]"
Temporal.PlainDate.from({ year: 2020, month: 1, day: 15, calendar: 'chinese' }).toString();
// => "2020-02-08[u-ca=chinese]" |
ah ok, that makes a lot of sense then - so it's only when 1) using a MonthDay, 2) a non-ISO calendar, and 3) serializing to string that you'd even see the arbitrary year? It does kind of seem like preserving the year provided to |
You'll also see it when calling Otherwise you're correct.
Yep, it's a tradeoff between deterministic serialization and somewhat-less-surprising (but still surprising!) serialization for instances using non-ISO calendars. We felt that the former was more important given that the primary string-serialization use case is for machine-readability, not human-readability. |
ok, I filed
#1614 before and there are several different point in that, and one of the piont got move to a more general case for discussion. But I have a specific issue of Temporal.Calendar.monthDayFromFields regarding the reference year.
Let's say we have the following code
What should be out1? (I am just use "iso8601" below for an object of iso8601 calendar, and that part is not my issue.
What should be out2?
Make up your mind BEFORE you read MY answer below first.
From my understanding of the current spec, the answer is
and I think that is very very strange. Notice both output isoYear as 1972 even the input is with year: 2021. But more strangely is one will output isoDay as 28 but the other as 29, while both of the input are having day:29.
Please let me know if you think my calcualted answer B and D is wrong after you carefully walk through the current spec with tese input values. Maybe I misssed some step that I didn't realize.
@sffc @justingrant @ptomato @ryzokuken @ljharb @Ms2ger
I have hard time to comprehance the design of such differences. Is this intentional and have discussed by champions already?
The text was updated successfully, but these errors were encountered: