Temporal edits batch 2 #37978
Temporal edits batch 2 #37978
Conversation
…s directly I think I understand what this was getting at, but I find it confusing. I'd suggest recommending fromEpochMilliseconds/fromEpochNanoseconds/the constructor to people asking the question of how to set these properties directly.
In the proposal docs and other materials that we created while developing
the proposal, the intention was to prefer plural units for durations, and
singular units for other types (even though both are accepted in both
cases.)
So,
duration.round('seconds')
plaindatetime.round('second')
This is in parallel with the accessor properties on each type.
github-actions
bot
added
Content:JS
| @@ -59,7 +59,7 @@ console.log(d3.toString()); // "PT10M" | |||
| const d1 = Temporal.Duration.from({ days: 1 }); | |||
| const d2 = Temporal.Duration.from({ months: 1 }); | |||
|
|
|||
| d1.add(d2); // RangeError: can't compare durations when "relativeTo" is undefined | |||
There was a problem hiding this comment.
This is Firefox's error message, which is currently the only error message readers can actually see in a browser.
There was a problem hiding this comment.
Ah, okay. What's the style guide on the text of illustrative error messages in MDN? I think Firefox's error message doesn't make sense here.
There was a problem hiding this comment.
I think we typically use Firefox's messages, especially for older APIs for historical reasons. But the baseline is that it has to be a real error message in a JS engine.
There was a problem hiding this comment.
Ladybird browser's error message is "Largest unit must not be a calendar unit", would that be OK?
There was a problem hiding this comment.
Actually I think the polyfill's message is fine too, if you think the error message is bad. Perhaps file a bug for SM too?
There was a problem hiding this comment.
| @@ -25,7 +25,7 @@ since(other, options) | |||
| - `other` | |||
| - : A string or a {{jsxref("Temporal.Instant")}} instance representing an instant to subtract from this instant. It is converted to a `Temporal.Instant` object using the same algorithm as {{jsxref("Temporal/Instant/from", "Temporal.Instant.from()")}}. | |||
| - `options` {{optional_inline}} | |||
| - : An object containing the options for {{jsxref("Temporal/Duration/round", "Temporal.Duration.prototype.round()")}}, which includes `largestUnit`, `roundingIncrement`, `roundingMode`, and `smallestUnit`. `largestUnit` and `smallestUnit` only accept the units: `"hour"`, `"minute"`, `"second"`, `"millisecond"`, `"microsecond"`, `"nanosecond"`, or their plural forms. For `largestUnit`, the default value `"auto"` means `"second"` or `smallestUnit`, whichever is greater. For `smallestUnit`, the default value is `"nanosecond"`. | |||
| - : An object containing the options for {{jsxref("Temporal/Duration/round", "Temporal.Duration.prototype.round()")}}, which includes `largestUnit`, `roundingIncrement`, `roundingMode`, and `smallestUnit`. `largestUnit` and `smallestUnit` only accept the units: `"hours"`, `"minutes"`, `"seconds"`, `"milliseconds"`, `"microseconds"`, `"nanoseconds"`, or their singular forms. For `largestUnit`, the default value `"auto"` means `"seconds"` or `smallestUnit`, whichever is greater. For `smallestUnit`, the default value is `"nanoseconds"`. | |||
There was a problem hiding this comment.
Out of curiosity is there a reason for this? I understand for Duration if you want to align with the property names, but these date/time objects' properties are singular anyway. The singular is also one fewer keystroke.
There was a problem hiding this comment.
It's because the largestUnit and smallestUnit refer to the properties of the returned Duration, not the Instants that are being subtracted. I think we have some discussion recorded somewhere in the Temporal repo about where we preferred singular and plural. I'll search for it and add a link here.
There was a problem hiding this comment.
| @@ -43,7 +43,7 @@ See the [`Intl.DateTimeFormat()` constructor](/en-US/docs/Web/JavaScript/Referen | |||
|
|
|||
| A string representing the given date-time according to language-specific conventions. | |||
|
|
|||
| In implementations with `Intl.DateTimeFormat`, this is equivalent to `new Intl.DateTimeFormat(locales, options).format(dateTime.toInstant())`, where `options` has been normalized as described above. | |||
| In implementations with `Intl.DateTimeFormat`, this is equivalent to `new Intl.DateTimeFormat(locales, { ...options, timeZone: dateTime.timeZoneId }).format(dateTime.toInstant())`, where `options` has been normalized as described above. | |||
There was a problem hiding this comment.
This is already implied by the "options has been normalized as described above" part, but okay if you want to make it explicit.
There was a problem hiding this comment.
I did think it was clearer to make it explicit, but I don't have any strong feelings about it. Up to you; feel free to drop this commit if you prefer it the way it was.
| @@ -28,7 +28,7 @@ Temporal.PlainYearMonth.compare(yearMonth1, yearMonth2) | |||
|
|
|||
| ### Return value | |||
|
|
|||
| Returns `-1` if `yearMonth1` comes before `yearMonth2`, `0` if they are the same, and `1` if `yearMonth2` comes after. They are compared by their underlying date values, ignoring their calendars. | |||
| Returns `-1` if `yearMonth1` comes before `yearMonth2`, `0` if they are the same, and `1` if `yearMonth2` comes after. They are compared by their underlying date values (usually the first day of the month), ignoring their calendars. | |||
There was a problem hiding this comment.
I'm not sure if this parenthetical helps, because for cross-calendar comparisons the date may be different anyway.
There was a problem hiding this comment.
For cross-calendar comparisons it's still the first day of each (calendar) month, no? It's just that the months may not be in the same calendar.
Description
A couple of minor edits to the Temporal docs: a few clarifications and typo fixes, and a suggestion to consistently use plural units for Duration and singular units for other types.
Motivation
I was asked to pitch in to help review the original Temporal docs PR, but didn't have time. So I'm going over it now and fixing anything that comes to mind. This is the result of one session of doing that.
Additional details
Related issues and pull requests
Relates to #37344 and #37905