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

Is daysInMonth the count of all days or the number of the last day? #1315

Closed
justingrant opened this issue Jan 20, 2021 · 18 comments · Fixed by #2446
Closed

Is daysInMonth the count of all days or the number of the last day? #1315

justingrant opened this issue Jan 20, 2021 · 18 comments · Fixed by #2446
Assignees
Labels
documentation Additions to documentation normative Would be a normative change to the proposal spec-text Specification text involved
Milestone

Comments

@justingrant
Copy link
Collaborator

Days can be skipped, either by calendars (e.g. Julian-to-Gregorian transition) or by time zone changes (e.g. Samoa's 2011 change to the other side of the International Date Line.

How should daysInMonth be affected by these skipped days?

@ljharb
Copy link
Member

ljharb commented Jan 20, 2021

I think I would assume that daysInMonth === days.length, where days is an array of every day in the month?

In fact, it might be a nice convenience API for a Calendar, or anything with a Year and a Month, to provide the equivalent of:

* days(month) {
  for (let i = 1; i <= daysInMonth; i += 1) {
    yield i;
  }
}

(i know this isn't accurate but it should convey the gist)

That way, nobody would be tempted to use daysInMonth to write a naive and incorrect implementation, when what they really want is the above, for a datepicker or similar.

@justingrant
Copy link
Collaborator Author

We discussed this issue in today's meeting. The consensus was that daysInMonth should be the number of the largest day of the month, because the other option (the count of actual days in the month) had problems:

  • Days skipped by time zones are invisible to Temporal types other than ZonedDateTime, which would mean that PlainDate.prototype.daysInMonth would return a different value depending on the Temporal type used. That seems weird. Also, including TimeZone in the daysInMonth calculation would add complexity and performance overhead to handle a very rare case. However, it would also be confusing for developers if days skipped because of time zones were treated differently from days skipped because of calendars.
  • daysInMonth is the current solution for the "what's the last day of this month?" use case. Given how rare it is to skip days (e.g. once in one time zone, and AFAIK never in the last 200 years in all ICU calendars) expanding the Temporal surface area to add a separate property for this rare case seems like a bad tradeoff.

Renaming this property (e.g. to lastDayOfMonth) might be a compromise option. We didn't discuss this in the meeting but I'd be open to it.

@ptomato
Copy link
Collaborator

ptomato commented Jan 21, 2021

I don't think we should rename it at this point unless the current name is really found to be problematic in delegate reviews.

Other than that, good summary, thanks!

@Louis-Aime
Copy link

I agree with all this. The Julian-to-Gregorian transition leads to very funny cases, like 1700 for most Protestant German states. This year of only 355 days was at the same time a leap year (in the beginning) and a common year (since 1 March). February had only 18 days (18 February was followed by 1 March). In reality, the "year" and the "month" are not the same before and after the transition. We switch from one calendar to another, which happen to share the same month names.

@Louis-Aime
Copy link

In the experimental development of a class of Western calendars, i.e. calendars that have a Julian-to-Gregorian transition date, I finally took the convention that daysInMonth is the number of days in a month, a similarly daysInYear the final number of days in the civil year, dayOfYear the rank of the current date since the first day of the same year.

This seems consistent with the names of the properties and other proposals like #729 ( transferred to Temporal.PlainYearMonth.atEndOfMonth method ).

Examples:

  • In Rome, the year 1582 had only 355 days, and october 1582 had 21 days instead of 31.
  • In several protestant states of Germany, 1700 had only 255 days. February 1700 had 18 days, and it happens that 18 Feb. was the eve of 1 March. In this year, 1 March was the 50th day of the year, not the 60th nor the 61th.
  • In parts of Switzerland, 1701-01-12 was the first day of 1701. The eve was 31 Dec. 1700 (julian).

On the other hand, I make the week numbering system changes abruptly at the switching date. The current's week number is increased by one the day of switching. In 1701 in Switzerland, weeks are counted is if the year had begun on 1 Jan. This convention can be changed.

If well documented, theses cases should not be a problem. They remain exceptions.

@ptomato ptomato added the documentation Additions to documentation label Feb 18, 2021
@ptomato ptomato added this to the Next milestone Feb 18, 2021
@ptomato ptomato modified the milestones: Next, Stage "3.5" Dec 8, 2022
@ptomato
Copy link
Collaborator

ptomato commented Dec 8, 2022

We should make a note about this in CalendarDateDaysInMonth (and maybe ISODaysInMonth)

@ptomato ptomato added the spec-text Specification text involved label Dec 8, 2022
@gibson042
Copy link
Collaborator

Is this not already covered by CalendarDateDaysInMonth (bold emphasis mine)?

It takes a date-like object date and returns the number of days in the given month in the calendar represented by calendar.

@justingrant
Copy link
Collaborator Author

@gibson042 per #1315 (comment), this definition creates some challenges. Using a definition of "1-based number of the last day of the month" might be a more developer-friendly definition. What do you think?

@gibson042
Copy link
Collaborator

That would be a normative change. The operation is specified to return the count of days, not the number associated with the last day (which would be discovered by a different approach, such as "constrain").

gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Dec 8, 2022
...rather than e.g. the number of the last day.

Fixes tc39#1315
@ptomato
Copy link
Collaborator

ptomato commented Dec 8, 2022

Oops, I completely missed that the comment linked by Justin (#1315 (comment)) actually does say that we decided to make this a normative change. That fell off my radar completely.

Note that this has an effect in PlainYearMonth arithmetic, see step 9 of AddDurationToOrSubtractDurationFromPlainYearMonth.

@justingrant
Copy link
Collaborator Author

I added the tag to put it to the agenda for next meeting so we can decide how to handle this. My fault for not catching this earlier!

@gibson042
Copy link
Collaborator

Hmm, also relevant for the discussion will be daysInWeek and daysInYear.

@gibson042
Copy link
Collaborator

Observation: the current text consistently specifies a count of days for all "xInY" properties.

Given that, adjusting daysInMonth to return something other than a count seems ill-advised. It may make sense to introduce a new property such as lastDayOfMonth, but @justingrant is concerned that daysInMonth would still be confused for it (especially since there appear to be no current CLDR or ICU calendars with skipped days)—if so, perhaps the existing properties should be renamed to clarify (e.g., by adding a prefix: countOfDaysInWeek, countOfDaysInMonth, etc.).

@justingrant
Copy link
Collaborator Author

In current Chrome, three Gregorian-derived ICU calendars (buddhist, japanese, and roc) do not use the proleptic Gregorian calendar. The gregory calendar uses proleptic. Two years ago I filed a bug (https://bugs.chromium.org/p/chromium/issues/detail?id=1173158) to resolve this inconsistency. IMO all Gregorian-derived calendars should either all use proleptic or all use non-proleptic.

This bug is still unresolved, but feedback from @Louis-Aime in that bug was that we should use proleptic from all of them.

@sffc does ICU4X have this inconsistency too?

If this bug is resolved to make them all proleptic, then AFAICT there will be no skipped days in any ICU calendars so I assume that no normative changes are needed.

Is it worth adding a note to the user-facing documentation of ZDT.p.daysIn* to clarify that those values come from the calendar, ignoring any days skipped or repeated due to time zone changes? For built-in time zones, this only affects one day in one time done (one day in 2011 in one time zone, Pacific/Apia), so this may be overkill.

@ptomato
Copy link
Collaborator

ptomato commented Jan 5, 2023

My opinion from the meeting:

  • I'd prefer to change nothing, and certainly not add any new APIs.
  • Use cases would be helpful.
  • To get the count of days in a month, if there is no API for it:
    const startOfMonth = date.with({ day: 1 });
    const countOfDaysInMonth = startOfMonth.until(startOfMonth.add({ months: 1 }), { largestUnit: 'days' }).days
  • To get the 1-based index of the last day in a month, if there is no API for it:
    const indexOfLastDay = date.with({ day: 1 }).add({ months: 1 }).subtract({ days: 1 }).day

ptomato pushed a commit that referenced this issue Jan 6, 2023
...rather than e.g. the number of the last day.

Fixes #1315
@ptomato ptomato reopened this Jan 6, 2023
@ptomato ptomato added the normative Would be a normative change to the proposal label Jan 12, 2023
@justingrant
Copy link
Collaborator Author

justingrant commented Jan 12, 2023

Meeting 2023-01-12:

  • daysInMonth and other *In* properties will be specified as the count, not the index of last one
  • We'll need to review the spec to make sure that we don't assume the other meaning anywhere, e.g. date arithmetic

cjtenny added a commit that referenced this issue Jan 19, 2023
…st day

Updates PlainYearMonth arithmetic accordingly and clarifies language around non-ISO calendar operations.

Fixes #1315
cjtenny added a commit that referenced this issue Jan 19, 2023
…st day

Updates PlainYearMonth arithmetic accordingly and clarifies language around non-ISO calendar operations.

Fixes #1315
@justingrant
Copy link
Collaborator Author

Should we add a note in the docs of ZonedDateTime that the *In* properties ignore any days that were skipped or repeated by the time zone? So it's not really the count of actual days in that time zone, it's a count of days in the calendar.

@ptomato
Copy link
Collaborator

ptomato commented Feb 7, 2023

I could go either way, it's an extraordinarily rare situation so it doesn't seem like it needs to be documented.

@justingrant justingrant self-assigned this Feb 7, 2023
guijemont added a commit to guijemont/test262 that referenced this issue Apr 25, 2023
guijemont pushed a commit to guijemont/proposal-temporal that referenced this issue Apr 25, 2023
…st day

Updates PlainYearMonth arithmetic accordingly and clarifies language around non-ISO calendar operations.

Fixes tc39#1315
guijemont pushed a commit that referenced this issue Apr 26, 2023
…st day

Updates PlainYearMonth arithmetic accordingly and clarifies language around non-ISO calendar operations.

Fixes #1315
guijemont pushed a commit that referenced this issue Apr 26, 2023
…st day

Updates PlainYearMonth arithmetic accordingly and clarifies language around non-ISO calendar operations.

Fixes #1315
guijemont added a commit to guijemont/test262 that referenced this issue Apr 26, 2023
…f days in a month

This change affects how PlainYearMonth's add and subtract behave. They
now ignore daysInMonth reimplementations in the calendar, and they call
dateadd() a different number of times.

See tc39/proposal-temporal#1315
ptomato pushed a commit to guijemont/test262 that referenced this issue Apr 28, 2023
…f days in a month

This change affects how PlainYearMonth's add and subtract behave. They
now ignore daysInMonth reimplementations in the calendar, and they call
dateadd() a different number of times.

See tc39/proposal-temporal#1315
ptomato pushed a commit to tc39/test262 that referenced this issue Apr 28, 2023
…f days in a month

This change affects how PlainYearMonth's add and subtract behave. They
now ignore daysInMonth reimplementations in the calendar, and they call
dateadd() a different number of times.

See tc39/proposal-temporal#1315
ptomato pushed a commit that referenced this issue Apr 28, 2023
…r of the last day

Updates PlainYearMonth arithmetic accordingly and clarifies language
around non-ISO calendar operations.

Fixes #1315

Co-authored-by: Guillaume Emont <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants