sffc commented 4 years ago

In Intl.DateTimeFormat, Intl.NumberFormat, Intl.DisplayNames, and even Temporal.DateTime, the date/time fields are singular: "second", "minute", "hour", "day", etc.

However, in Temporal.Duration, they are plural: "seconds", "minutes", etc.

I understand the reasoning, that it sounds more natural in English to say "seconds: 30" instead of "second: 30", but there are some downsides to the inconsistent naming:

Users may want to use the same key as input to both Temporal.Duration and one of the other classes
Non-native English speakers who don't care about language fluency have to learn to use singular in some cases and plural in others

Thoughts on making the Temporal.Duration field names singular?

ljharb commented 4 years ago

Alternatively, could they all be made plural? Singular only makes sense in english with a value of 1.

sffc commented 4 years ago

If we were doing this new, I'd say they could all be plural, but at least Intl.DateTimeFormat is already stable and uses singular names.

gibson042 commented 4 years ago

This was the first open question of #120, so I guess not everything there was dealt with before it closed (and the rest should probably be audited as well). But as for this specific issue, I'm really not sure if it makes sense for Duration to be different... @sffc, can you provide an example where an author would want to use the same key as input to both Temporal.Duration and one of the other classes?

littledan commented 4 years ago

For RelativeTimeFormat, we decided to permit both singular and plural. In my personal opinion, plural makes sense here.

ptomato commented 4 years ago

Would anyone still like to discuss changing the field names or can we close this?

sffc commented 4 years ago

Maybe we could just accept both singular and plural everywhere, on input and output. Then you can't make a typo because both forms work the same.

ryzokuken commented 4 years ago

I too agree that we should use plural. I'm not convinced that we should allow both, as opposed to emitting better errors and better documentation.

littledan commented 4 years ago

The idea from here will be to ship the polyfill accepting just the plural form, and see if we get feedback asking for the single form. We'll revisit this issue before Stage 3.

ptomato commented 4 years ago

615 is feedback on this issue.

justingrant commented 4 years ago

Having different singular vs. plural is actually very helpful, because it lets Temporal throw exceptions when a developer passes a DateTime/Date/Time/etc instead of a Duration, or vice versa. Of course, Temporal would actually have to start throwing those errors which was my suggestion in #615.

Suggestion: can Temporal throw in cases when a singular field name is expected but the plural variant is supplied, or vice versa? Or when an XxxxLike object is passed to a Temporal object constructor?

The only thing I think we shouldn't do is ship the current behavior, where only plurals are accepted but no exceptions are thrown if you pass the wrong type, so you get zeroed-out values without any complaints from Temporal. I wasted 10 minutes the other day wondering why I couldn't add minute: 10 before I noticed the missing "s". ;-)

Another benefit of different naming is that TypeScript users will catch the "pass wrong type" problem in the compiler or even right in the IDE, without having to wait to trigger the exception at runtime. So once we have TS types, my minute: 10 bug above would be an obvious in-IDE error.

sffc commented 4 years ago

We don't throw in 402 if there are unknown options in the options bag. There are pros and cons of doing it either way. However, the 402 options bag carries options, not data, which is an important distinction when we go about extending the options bags.

If we did throw here, would we specifically blacklist certain common misspellings, like "minute" instead of "minutes", or would we throw for any unrecognized option?

justingrant commented 4 years ago

I don't have enough context to know if there are valid use-cases for providing unknown field names in property bags.

My top priority would be to make sure that the plural vs. singular cases are caught because those errors will be painfully common.

We'd need to make sure that custom fields added by non-ISO calendars don't get flagged as typos.

justingrant commented 4 years ago

I thought about this more. I think passing "extra" properties seems relatively harmless (other than the obvious singular vs. plural cases above which I think should be blacklisted).

But passing a non-empty object with zero matching properties seems like it's almost guaranteed to be a mistake, e.g. passing a Time-like property bag when a Date-like is expected, or passing an Absolute when a DateTime property bag is expected. I've already written code with both of these bugs and they were frustrating to track down.

Should Temporal throw if zero fields in the bag match a whitelist of expected fields, including custom calendar fields? And if yes, would we be able to prevent throwing for {} in contexts where {} might be valid, like duration.with({}, {disambiguation: 'balance'})?

justingrant commented 4 years ago

Update: looks like Duration.prototype.with in the polyfill throws for {} and {foo: 1}, while other Duration methods don't throw. Seems like there should be consistent behavior unless there's a reason not to.

duration = Temporal.Duration.from('PT10H54M')
duration.plus({}).toString()
// => "PT10H54M"
duration.minus({}, {disambiguation: 'balance'}).toString()
// => "PT10H54M"
Temporal.Duration.from({}).toString()
// => "PT0S"
duration.with({}, {disambiguation: 'balance'}).toString()
// throws: Uncaught RangeError: invalid duration-like

ptomato commented 4 years ago

The reason was that from() should have the same optional/required semantics as the constructor arguments (and in the case of Temporal.Duration, all constructor arguments are optional) whereas in with(), it's probably an error if you passed an empty object.

I think rather than duration.with({}, {disambiguation: 'balance'}) a better way would be Temporal.Duration.from(duration, {disambiguation: 'balance'}).

sffc commented 4 years ago

Related: does it matter if the misspelled properties exist on the prototype but aren't owned properties?

const a = { hours: 2, minute: 30 };
const b = Object.create(a);
const d = Temporal.Duration.from(b);  // what should this be?

justingrant commented 4 years ago

@ptomato whereas in with(), it's probably an error if you passed an empty object.

Why is it an error for with but not for plus and minus?

I think rather than duration.with({}, {disambiguation: 'balance'}) a better way would be Temporal.Duration.from(duration, {disambiguation: 'balance'}).

The challenge is that you need to go back to the start of a chain of methods to insert Temporal.Duration.from:

Ergonomics: it's many more keystrokes, including both the method call and the the cursor movement to go to the start of the chain and back again to insert the options bag.
If a method chain is long, it can be hard to figure out exactly where to cut the chain to insert the Temporal.Duration.from, especially if the chain is interleaved with non-Duration methods.
When patching existing code, adding Temporal.Duration.from requires inserting code in two places, instead of appending to a chain which puts all new code in one place. Chaining -> cleaner patches.
With from, IDE users can't use autocomplete to discover the next chained method to use. With with, it's easier: the user can scroll through autocomplete until they find the correct method to use.

All that said, I think both with and from are bad if all that's needed is balancing. And the current chainable solution, .minus({}, {disambiguation: 'balance'}, feels like an awful hack. IMHO a better solution (discussing in #645) could be to make balancing easier & more consistent so that there'd never be a need to pass an empty object with, plus, or minus so all three could throw.

justingrant commented 4 years ago

@sffc Related: does it matter if the misspelled properties exist on the prototype but aren't owned properties?

Non-owned properties for property bag objects are a pretty advanced use case that almost all developers will not use, so I don't have a strong opinion on this one way or the other.

gibson042 commented 4 years ago

I would propose throwing an error when both no expected fields are present and one or more cross-type fields are present, and I don't think own vs. prototype position should matter:

Let result be a new Record with all the internal slots given in the Internal Slot column in \\.
Let empty be true.
For each row of \\, except the header row, in table order, do
1. Let slotName be the Internal Slot value of the current row.
2. Let fieldName be the Property value of the current row.
3. If ? HasProperty(temporalDurationLike, fieldName) is true, then
  1. Set empty to false.
  2. Let val be ? Get(temporalDurationLike, fieldName).
  3. Set result.[[\<_slotName_>]] to ? ToInteger(val).
If empty is true, then
1. For each row of \\, except the header row, in table order, do
  1. Let fieldName be the Property value of the current row.
  2. If ? HasProperty(temporalDurationLike, fieldName) is true, then
    1. Throw a TypeError exception.

This way Temporal.Duration.from({}) will generate a zero duration but Temporal.Duration.from({hour: 1}) will fail.

justingrant commented 4 years ago

@gibson042 I would propose throwing an error when both no expected fields are present and one or more cross-type fields are present, and I don't think own vs. prototype position should matter

What about Temporal.Duration.from({hours: 1, minute: 1})? My inclination would be to throw in that case.

gibson042 commented 4 years ago

Hmm, so that would be more like "throw if you see any Temporal.DateTime fields"? Yeah, that can work. But I don't think I'd go pairwise, where Temporal.Duration.from({hours: 1, minutes: 1, minute: 0}) doesn't throw.

justingrant commented 4 years ago

But I don't think I'd go pairwise, where Temporal.Duration.from({hours: 1, minutes: 1, minute: 0}) doesn't throw.

Sounds good to me, although that seems like an obscure case so I think it'd also be OK to throw in the case you listed, which might make the blacklist easier for browsers to implement. But I think either way would be OK as long as the common single-typo case (Temporal.Duration.from({hours: 1, minute: 1})) throws.

sffc commented 4 years ago

Did we eliminate adopting the Intl.RelativeTimeFormat behavior of allowing the keys to be either singular or plural? I only see this comment from @justingrant, which is valid, but I don't know if it eliminates the option:

Having different singular vs. plural is actually very helpful, because it lets Temporal throw exceptions when a developer passes a DateTime/Date/Time/etc instead of a Duration, or vice versa. Of course, Temporal would actually have to start throwing those errors which was my suggestion in #615.

Non-owned properties for property bag objects are a pretty advanced use case that almost all developers will not use, so I don't have a strong opinion on this one way or the other.

No developer would actually write the code I posted, but checking for properties up the prototype chain is absolutely something that can and will happen in the real world. For example, maybe someone writes a third-party duration class for interacting with their database. The fields like "years" and "days" might be getters living on the prototype.

If we go with the rejection approach, do we scope the problem to singular/non-singular, or do we also handle misspellings, like hors or minnutes or wekks?

justingrant commented 4 years ago

If we go with the rejection approach, do we scope the problem to singular/non-singular, or do we also handle misspellings, like hors or minnutes or wekks?

The former only. The goal isn't to catch every typo, it's to avoid cases where developers accidentally/mistakenly use one set of fields when they should be using the other.

justingrant commented 4 years ago

My current opinion:

Accept both singular and plural everywhere units are accepted, like Intl.RelativeTimeFormat https://github.com/tc39/proposal-temporal/issues/325#issuecomment-638557552
Emit (and document!) plural for duration fields and singular everywhere else
Temporal methods should always throw if a Duration instance is passed where a non-Duration object is expected, or if a non-Duration instance is passed where a DurationLike object is expected. This might already be the current behavior; not sure.

ptomato commented 4 years ago

@justingrant How do you feel about the TypeScript bindings? Should they accept both? My feeling is that they should not.

I agree with what you proposed there. It does leave a weird case where you can do dateTime.plus(otherDateTime.getFields()) or dateTime.difference(duration.getFields()) but as far as I can tell, there is no way to prevent that and still support both singular and plural fields.

justingrant commented 4 years ago

How do you feel about the TypeScript bindings? Should they accept both? My feeling is that they should not.

@ptomato Hmm, good question. The more I researched the TS implications, it's made me open to changing my opinion about what we should do here. The problem is that TS doesn't seem to have a concept of "allowed but not preferred". AFAIK there's no ability to control, for example, whether the TS compiler emits a warning or an error. Nor does there seem to be a way in VSCode to prevent it from suggesting all allowed variants of a property name. So if you want to prevent date.plus({hour: 1}) from causing a TS compiler error, then I don't think you can also prevent VSCode from showing both variants to the user in autocomplete:

I asked a question in Stack Overflow to see if there's a way to hide the "wrong" variant from autocomplete but not trigger a compiler error either if the wrong variant is used. https://stackoverflow.com/questions/63837660/in-typescript-and-or-jsdoc-how-to-indicate-that-some-property-names-in-a-record

If we go the route that you're recommending and TS will show a compiler error if the wrong variants are used, is this OK? If we do this then should we just prohibit the wrong variants at runtime too so that runtime and TS won't disagree on what's valid code?

I agree with what you proposed there. It does leave a weird case where you can do dateTime.plus(otherDateTime.getFields()) or dateTime.difference(duration.getFields()) but as far as I can tell, there is no way to prevent that and still support both singular and plural fields.

There is one case I think this would be a feature not a bug: converting Duration to Time and vice versa, which is a fairly common use case.

const time = Temporal.Time.from('00:15');
const duration = Temporal.Duration.from(time.getFields());
const timeRoundTrip = Temporal.Time.from(duration.getFields());

ptomato commented 4 years ago

I think it's OK for compile time and runtime to disagree as long as compile time is stricter. (But as I'm not an experienced TypeScripter I don't know if this is usually done differently.)

I can think of another example where this happens. For example in Temporal.Date.from, the type of the first argument is Temporal.Date | DateLike | string, but at runtime it is actually any. If you pass a boolean, number, symbol, bigint, etc., it's converted to a string and then the string is parsed as an ISO string. I don't think it would be useful for the type to actually be any in the .d.ts file.

I don't think it would be a great loss if the TypeScript compiler were to flag the conversion between Duration.from(time) and Time.from(duration)`. What are the use cases you had in mind? I can only think of ones where it seems like the Time should already have been a Duration in the first place.

justingrant commented 4 years ago

I'm mostly worried about the case where valid JS code breaks for a mainstream use case when it's run through the TS compiler. If people get used to being lazy about pluralization, it will put us in a bind: do we expand the TS types to include the "wrong" variants (and degrade autocomplete usability) or do we accept that lots of valid JS code won't be valid when run through the TS compiler.

So I guess it depends on whether we think a lot of users will start depending on this behavior. It's a slippery slope, and the hard part is that if people start relying on the "wrong" variants then there's no good way to put the TS horse back in the barn.

So now I'm back to my previous opinion, rolling up other feedback above too:

In methods accepting a duration, throw if any property name matches a singular unit name
In methods accepting a duration, throw if a non-Duration temporal type instance is provided.
In methods accepting a non-duration, throw if any property name matches a plural unit name.
In methods accepting a non-duration, throw if a Duration temporal type instance is provided.
Only use a fixed list of known units for the ISO calendar. Non-ISO fields are exempted from these checks.
Don't bother with any other checks (mis-spelling, etc.)
Handle non-owned properties however we think will be best, which per @sffc above I think means that the checks above should include non-owned properties.

justingrant commented 4 years ago

Decision 2020-09-11:

Property Names

All methods in Temporal that accept a TimeLike or a DurationLike property bag must include at least one correctly-spelled property.
Empty objects will throw, as will objects that only contain the wrong pluralization and/or mis-spelling.
Cases like {years: 1, hour: 1} won't throw because it's obscure and obvious to solve via debugging.

Unit names in options values (e.g., smallestUnit)

Allow both variants.
Use JSDoc @deprecated for the "wrong" unit (the one differing from the type's field name) in TypeScript types.
Ideally, we'd have an ESLint rule to catch the "wrong" variant, but that's out of scope.

ptomato commented 4 years ago

As far as I can tell, the APIs affected by this change are:

from, with, add, subtract of Date, DateTime, Duration, Time, YearMonth
Temporal.Duration.prototype.round (via relativeTo)
Temporal.MonthDay.from
Temporal.MonthDay.prototype.with
Temporal.Instant.prototype.add
Temporal.Instant.prototype.subtract

I'd also propose disallowing "week" since there is no property named that anywhere.

FrankYFTang commented 3 years ago

Please use singular form in Duration for the unit to make it consistent with ECMA402 Intl.NumberFormat. Notice I am NOT talking about Intl,.DateTimeFormat nor Intl.RelativeTimeFormat here. I am talking about "Table 2: Simple units sanctioned for use in ECMAScript" in https://www.ecma-international.org/wp-content/uploads/ECMA-402.pdf which was published in Jun 2020 already. Using plural form of "years", "months", "weeks", "days", "seconds", "minutes", "seconds", "milliseconds" make it inconsistent with Intl.NumberFormat . All the units in Intl.NumberFormat are expressed in singular form, not plural form.

The unit in the phrase "5 days" is "day". => is a correct English sentence. The unit in the phrase "5 days" is "days". => is an incorrect English sentence.

ptomato commented 3 years ago

I think the naming of the Duration fields can be seen as completely separate from the unit option in NumberFormat. The rationale we had for using the plural form for the names of the fields is that it's correct to say "a duration of 5 minutes", not "a duration of 5 minute", similarly Temporal.Duration.from({ minutes: 5 }) and time.add({ minutes: 5 }), not Temporal.Duration.from({ minute: 5 }) or time.add({ minute: 5 }).

FrankYFTang commented 3 years ago

How would Temporal.Duration.from({ minutes: 1 }) or time.add({ minutes: 1 }) make sense then?

FrankYFTang commented 3 years ago

I think the naming of the Duration fields can be seen as completely separate from the unit option in NumberFormat.

Why? Both are UNIT, right? in particular, later in Intl.DurationFormat.prototype.formatToParts and Intl.DurationFormat.prototype.resolvedOptions we need to output the part and need to generate the unit Currently Intl.NumberFormat.prototype.resolvedOptions() generate singular form of "year", "month", "week", "day", "second", "minute", "second", "millisecond" as "unit" in the output. If Temporal decide to use the plural form it is forcing the Intl.DurationFormat proposal to either A. generate output string of unit inconsistent with Temporal.Duration OR B. generate output string of "unit" inconsistent within ECMA402

And this is totally avoidable.

ptomato commented 3 years ago

First, I don't agree with your point that the name of a unit is always required to be singular in English and that everything else is incorrect. It just doesn't match with my experience speaking about physical units.

Second, I don't agree that this is forcing Intl.DurationFormat.formatToParts to output the same names. If type: "unit" entries in the parts array have a precedent of being singular, then I don't think it's inconsistent with Temporal.Duration to make them singular. The name of a Temporal.Duration field like weeks means the number of weeks in the duration, not the unit the duration is in, so I don't agree that both are units. I don't believe it's likely that callers of formatToParts would take the type: "unit" entries and try to feed the unit strings back into Temporal.Duration.from or some similar function as property names in a property bag.

Third, if you want to debate about what is correct English, in my opinion usages like time.add({ minute: 5 }) and date.add({ day: 5 }) sound much worse than any of the English usages that you are objecting to. Specifically, they sound worse than time.add({ minutes: 1 }) and date.add({ days: 1 }) even if those are technically incorrect, because ever since named variables were invented programmers have been accustomed to using variable names like files or bytesUsed or whatever, and dealing with the possibility that they can be equal to 1. The reverse is not true, we don't make variables like file = 42 or byteUsed = 1e9.

sffc commented 3 years ago

To me, the most compelling argument is consistency between input and output of Intl and Temporal. I don't disagree about the "readability" of the plurals, but that doesn't weigh as highly for me as consistency. This is why I had originally opened this issue last year.

However, we did already discuss this issue, the champions already decided to move forward with plurals, despite my mild opposition, and Temporal is already Stage 3. So any changes need to be compelling.

FrankYFTang commented 3 years ago

First, I don't agree with your point that the name of a unit is always required to be singular in English

Could you cite any reference as evidence to support your point?

Here is my citation: https://www.bipm.org/metrology/time-frequency/units.html#:~:text=BIPM%20%2D%20SI%20unit%20of%20time%20(second)&text=%2C%20the%20unperturbed%20ground%2Dstate%20hyperfine,is%20equal%20to%20s%E2%80%931. "The second, symbol s, is the SI unit of time."

Notice, the text is NOT "The seconds, symbol s, is the SI unit of time." - it use the singular form to refer to the unit itself. https://www.nist.gov/pml/weights-and-measures/si-units-time "SI Units – Time The second (s) is defined by" .... Notice it is not referred as "The seconds (s) is defined by"

from NIST https://physics.nist.gov/cuu/Units/current.html " Unit of time | second | The second, symbol s, is the SI unit of time. It is defined by taking the fixed numerical value of the cesium frequency ΔνCs, the unperturbed ground-state hyperfine transition frequency of the cesium 133 atom, to be 9 192 631 770 when expressed in the unit Hz, which is equal to s-1."

Notice NIST also use the singular form while referring to the time unit, not using the plural form seconds.

https://physics.nist.gov/cuu/pdf/sp811.pdf#page=16 "Table 1. SI base units time second s " The unit listed in "Guide for the Use of the International System of Units (SI) " use the singular form- "second" not "seconds".

ISO8601 http://www.loc.gov/standards/datetime/iso-tc154-wg5_n0038_iso_wd_8601-1_2016-02-16.pdf "2.1.4 time scale Note 3 to entry: Customary time scales use various units of measurement in combination, such as second,
minute, hour, or various time intervals of the calendar such as calendar day, calendar month and calendar year." " Notice all the units were mentioned in the singular form, not plural form.

2.1.6 duration ... Note 4 to entry: The SI unit of duration is the second." Notie it is NOT "the seconds" but is "the second".

"2.2 Time units, nominal durations and time intervals 2.2.1 second base unit of measurement of time in the International System of Units (SI) as defined by the
International Committee of Weights and Measures (CIPM, i.e. Comité International des Poids et Mesures)"

All are referring as "second" not "seconds".

"2.2 Time units, nominal durations and time intervals" 2.2.3 minute unit of time

2.2.4 hour unit of time 2.2.5

day 〈unit of time〉 unit of time

2.2.6 calendar day

2.2.7 day 〈duration〉 duration of a calendar day

2.2.8 calendar week

2.2.9 week duration of a calendar week

2.2.11 calendar month

2.2.12 month

2.2.13 calendar year

2.2.14 year duration of 365 or 366 calendar days depending on... "

All of these standard use singular form to denote the time "unit".

Could you find some standard as reference as evidence to support the use of plural form?

FrankYFTang commented 3 years ago

Second, I don't agree that this is forcing Intl.DurationFormat.formatToParts to output the same names. If type: "unit" entries in the parts array have a precedent of being singular, then I don't think it's inconsistent with Temporal.Duration to make them singular.

I agree with what you said. It will only force Intl.DurationFormat.formatToParts to be either inconsistent with Temporal or inconsistent with Intl.NumberFormat but will not force it to output the same name.

FrankYFTang commented 3 years ago

The name of a Temporal.Duration field like weeks means the number of weeks in the duration, not the unit the duration is in, so I don't agree that both are units.

So.... A. Do you agree what got listed in "Table 12: Singular and plural units" https://tc39.es/proposal-temporal/#sec-temporal-singular-and-plural-units are units in Temporal?

B. Do you agree the value of "unit"., "smallestUnit" or "largestUnit" are unit in Temporal?

C. Is it your stand that the properties listed in "Table 7: Properties of a TemporalDurationLike" https://tc39.es/proposal-temporal/#table-temporal-temporaldurationlike-properties are NOT unit?

ptomato commented 3 years ago

Could you find some standard as reference as evidence to support the use of plural form?

Please stop flooding the discussion with those links. English usage is not standardized by a standards organization, so no, I do not intend to do that. Since in my experience singular is correct as well as plural, I have no doubt that you can find an arbitrary number of links with examples of singular unit usage, so I'm not going to get into a link bidding war.

FrankYFTang commented 3 years ago

The name of a Temporal.Duration field like weeks means the number of weeks in the duration, not the unit the duration is in, so I don't agree that both are units.

D. What is your stand of the return value and the the value of argument disallowedUnits, defaultUnit, fallback in 13.21 ToLargestTemporalUnit ( normalizedOptions, disallowedUnits, defaultUnit ) https://tc39.es/proposal-temporal/#sec-temporal-tolargesttemporalunit 13.22 ToLargestTemporalDurationUnit ( normalizedOptions ) https://tc39.es/proposal-temporal/#sec-temporal-tolargesttemporaldurationunit 13.23 ToSmallestTemporalUnit ( normalizedOptions, disallowedUnits ) https://tc39.es/proposal-temporal/#sec-temporal-tosmallesttemporalunit 13.24 ToSmallestTemporalDurationUnit ( normalizedOptions, disallowedUnits, fallback ) https://tc39.es/proposal-temporal/#sec-temporal-tosmallesttemporaldurationunit 13.25 ToTemporalDurationTotalUnit ( normalizedOptions ) https://tc39.es/proposal-temporal/#sec-temporal-totemporaldurationtotalunit

are the return value and the value in the argument disallowedUnits, defaultUnit, fallback from your point of view unit or "not unit"?

FrankYFTang commented 3 years ago

English usage is not standardized by a standards organization, so no, I do not intend to do that.

Then... could you tell me, from your point of view, WHO define the English usage?

ptomato commented 3 years ago

The name of a Temporal.Duration field like weeks means the number of weeks in the duration, not the unit the duration is in, so I don't agree that both are units.

So.... A. Do you agree what got listed in "Table 12: Singular and plural units" https://tc39.es/proposal-temporal/#sec-temporal-singular-and-plural-units are units in Temporal?

B. Do you agree the value of "unit"., "smallestUnit" or "largestUnit" are unit in Temporal?

Both yes, but because of (C) it is important to accept both forms to avoid bugs and confusion around something like Temporal.Duration.from({ years: 2, months: 6 }).round({ smallestUnit: 'years' }) where developers would otherwise fall into the trap of having to remember that the smallestUnit values are different from the field names.

C. Is it your stand that the properties listed in "Table 7: Properties of a TemporalDurationLike" https://tc39.es/proposal-temporal/#table-temporal-temporaldurationlike-properties are NOT unit?

As far as this is relevant, I guess the answer is yes. The goal here is to make an API that fits with the way that programmers think. I'm not intending to continue this line of discussion where you demand that I categorize everything into "units" or "not units".

FrankYFTang commented 3 years ago

you demand that I categorize everything into "units" or "not units".

First of all, I or anyone else never categorize anything into "units" or "not units" . It is you who bring up the concept of " I don't agree that both are units." which imply in your mind some words of "days" in Temporal are units while some other "days" in Temporal are NOT unit and that was a very new concept no body ever mentioned before. There are no way for me, or anyone else, to read your mind to figure which fall into unit and which does not, so I have to ask you to clarify such new concept from your point of view.

(C) it is important to accept both forms to avoid bugs and confusion around something like Temporal.Duration.from({ years: 2, months: 6 }).round({ smallestUnit: 'years' }) where developers would otherwise fall into the trap of having to remember that the smallestUnit values are different from the field names.

It is very easy to avoid confusion and bugs or fall into the trap if Temporal only use singular form. Then there are no need for the developer to remember anything beside using singular form...

FrankYFTang commented 3 years ago

The goal here is to make an API that fits with the way that programmers think.

How many programmers have you survey to conclude the use of plural forms fit better for the way that most programmers think but not just a small minority? If no survey have been conducted, is that more accurate to state "The goal here is to make an API that fits with the way that the majority of the champions of this proposal think."

justingrant commented 3 years ago

I have a suggestion that may be a middle path to resolve this problem. Frank is correct that there is an external-to-JS definition of units by SI. Those units are singular. But Philip is also correct that there are benefits for using plurals for duration property names. For example, plural properties on Duration makes it easier to know the type of the receiver when reading code, and makes it easier to catch cases where a Duration is used in place of a non-Duration object (or vice versa).

// I know `y` is not a Duration or DurationLike object...
b = y.year; 
// ...so this next line is probably correct
date = Temporal.PlainDateTime.from(y);

// I know `x` must be a Duration or DurationLike object...
a = x.years;
// ...so I know that this next line is a bug
time = Temporal.PlainDateTime.from(x);

If Duration property names are singular, then the bug above may not throw at runtime (if it's a plain object) nor would it be caught by TS or ESLint. And the bug would be much harder to spot in a code review.

Perhaps a way to address Frank's and Philip's concerns might be to acknowledge that there are two subtly different concepts here:

The name of a unit. This name exists outside JS and is defined by SI and elsewhere. Unit names are always singular.
The name of a property used in APIs and source code. It's common to pluralize APIs which accept or return a count or an array, e.g. minimumIntegerDigits in Intl or .children in DOM API.

With that distinction in mind, what if we declare that unit names (as strings) are always singular because they refer to the unit itself. But property names are singular if the unit is an index or plural if the unit is a count.

Specifically:

Anywhere we use the name of the unit as a string, it should be singular. This means that options objects like {smallestUnit: 'minute'} throughout Intl and Temporal would use singular unit names, including when the unit in question belongs to a Temporal.Duration.
Property names (Temporal getters, input to Temporal from methods, or input to Intl format methods) should match the meaning of the property. In cases where the property value is a count (e.g. Temporal.Duration), use a plural name for the property. In cases where the value is an index (e.g. Temporal.PlainTime) use a singular name.

This would make options consistent across Temporal and Intl without preventing the bug-catching ability noted above.

It'd also solve a usability issue that exists in the current Temporal API where pluralization varies not based on the receiver type but on the type of the output object, which is not obvious. Example: time.round({smallestUnit: 'hour'}) (output is a PlainTime) vs. time.since(y, {smallestUnit: 'hours'}) (output is a Duration). Making options always singular would simplify this mental gymnastics without removing the DX advantage noted above.

Anyway, I'd recommend adding this as Option 4 to discuss along with the others in https://docs.google.com/presentation/d/1MaPJ71tlFnRtcm7GpUAwH8nJ5jEUuSkm9rxt53RiAf8/edit#slide=id.gcc5ed40f56_0_54

FrankYFTang commented 3 years ago

Another issue is the name with plural form of internal slots in ECAM262 are all referring to "A List of X". I agree that is not true in ECMA402 as "minimumIntegerDigits", "minimumFractionDigits", "maximumFractionDigits", "minimumSignificantDigits", and "maximumSignificantDigits" does not follow that. If Temporal start to use plural form for names of internal slot, then it will violate the precedences established within ECMA262.

It's common to pluralize APIs which accept or return a count or an array, e.g. minimumIntegerDigits in Intl or .children in DOM API.

Please do not MIX "a count" AND "an array" together here. The "children" in the DOM API is returning an ARRAY, which I have no problem to use plural form for such usage. If "days" is used to accept an array of days, I will not argue to use singular form, but agree to use plural form. But the value here is a SINGLE number ,not a LIST of numbers, therefore, it should not use the plural form.

https://developer.mozilla.org/en-US/docs/Web/API/ParentNode/children

In cases where the property value is a count...., use a plural name for the property.

I disagree, I believe it should be

Only where the property value is a LIST or iterator, we then use a plural name for the property.

Precedences in ECMA262 23.1.3.4 Array.prototype.entries ( ) https://tc39.es/ecma262/#sec-array.prototype.entries

23.1.3.16 Array.prototype.keys ( ) https://tc39.es/ecma262/#sec-array.prototype.keys

23.1.3.32 Array.prototype.values ( ) https://tc39.es/ecma262/#sec-array.prototype.values

23.2.3.6 %TypedArray%.prototype.entries ( ) https://tc39.es/ecma262/#sec-%typedarray%.prototype.entries

23.2.3.16 %TypedArray%.prototype.keys ( ) https://tc39.es/ecma262/#sec-%typedarray%.prototype.keys

23.2.3.30 %TypedArray%.prototype.values ( ) https://tc39.es/ecma262/#sec-%typedarray%.prototype.values

24.1.3.4 Map.prototype.entries ( ) https://tc39.es/ecma262/#sec-map.prototype.entries

24.1.3.8 Map.prototype.keys ( ) https://tc39.es/ecma262/#sec-map.prototype.keys

24.1.3.11 Map.prototype.values ( ) https://tc39.es/ecma262/#sec-map.prototype.values

24.2.3.5 Set.prototype.entries ( ) https://tc39.es/ecma262/#sec-set.prototype.entries

24.2.3.8 Set.prototype.keys ( ) https://tc39.es/ecma262/#sec-set.prototype.keys

24.2.3.10 Set.prototype.values ( ) https://tc39.es/ecma262/#sec-set.prototype.values

In ECMA262, in cases where the property value is a count...., currently it use a singular name + "Length" for the property. For example: 25.1.5.1 get ArrayBuffer.prototype.byteLength https://tc39.es/ecma262/#sec-get-arraybuffer.prototype.bytelength

23.2.3.2 get %TypedArray%.prototype.byteLength https://tc39.es/ecma262/#sec-get-%typedarray%.prototype.bytelength

23.2.3.18 get %TypedArray%.prototype.length https://tc39.es/ecma262/#sec-get-%typedarray%.prototype.length

25.2.4.1 get SharedArrayBuffer.prototype.byteLength https://tc39.es/ecma262/#sec-get-sharedarraybuffer.prototype.bytelength

25.3.4.2 get DataView.prototype.byteLength https://tc39.es/ecma262/#sec-get-dataview.prototype.bytelength

FrankYFTang commented 3 years ago

Also, the places to use name of plural form for returning a LIST or an object in ECMA402

10.3.4 Intl.Collator.prototype.resolvedOptions ( ) https://tc39.es/ecma402/#sec-intl.collator.prototype.resolvedoptions

11.4.4 Intl.DateTimeFormat.prototype.formatToParts ( date ) https://tc39.es/ecma402/#sec-Intl.DateTimeFormat.prototype.formatToParts

11.4.7 Intl.DateTimeFormat.prototype.resolvedOptions ( ) https://tc39.es/ecma402/#sec-intl.datetimeformat.prototype.resolvedoptions

12.4.4 Intl.DisplayNames.prototype.resolvedOptions ( ) https://tc39.es/ecma402/#sec-Intl.DisplayNames.prototype.resolvedOptions

13.4.4 Intl.ListFormat.prototype.formatToParts ( list ) https://tc39.es/ecma402/#sec-Intl.ListFormat.prototype.formatToParts

13.4.5 Intl.ListFormat.prototype.resolvedOptions ( ) https://tc39.es/ecma402/#sec-Intl.ListFormat.prototype.resolvedoptions

15.4.4 Intl.NumberFormat.prototype.formatToParts ( value ) https://tc39.es/ecma402/#sec-intl.numberformat.prototype.formattoparts

15.4.5 Intl.NumberFormat.prototype.resolvedOptions ( ) https://tc39.es/ecma402/#sec-intl.numberformat.prototype.resolvedoptions

16.4.4 Intl.PluralRules.prototype.resolvedOptions ( ) https://tc39.es/ecma402/#sec-intl.pluralrules.prototype.resolvedoptions

17.4.4 Intl.RelativeTimeFormat.prototype.formatToParts ( value, unit ) https://tc39.es/ecma402/#sec-Intl.RelativeTimeFormat.prototype.formatToParts

17.4.5 Intl.RelativeTimeFormat.prototype.resolvedOptions ( ) https://tc39.es/ecma402/#sec-intl.relativetimeformat.prototype.resolvedoptions

littledan commented 3 years ago

As Shane has noted, Temporal is at Stage 3, so we should have a strong reason for changes. Consistency in itself is important, but it is the kind of quality that we should've ideally made a final call on before Stage 3, given that all of these arguments were on the table. I'm not really sure what changed. I would prefer that we consider Stage 3 proposals stable.

I'm not so sympathetic to the absolute arguments in each direction. We're defining a developer interface here, so I think it's important to meet developer intuition here. There are intuitions in both directions for why singular or plural makes sense. But we discussed this above, both in this issue and in Temporal champion group calls, including with members of Frank's and Craig's team, and we concluded on a direction.

@justingrant's suggestion above makes sense to me as a simple compromise way through while retaining the common developer intuition of plural, and without violating the stability of the proposal. This design would match the earlier decision on Intl.RelativeTimeFormat accepting both singular and plural for basically the same reasons. I'd support that change.

However, I don't expect to agree to consensus on simply replacing plural with singular, given that it is a breaking change, not based on new information and had already been discussed openly with ample time for feedback.

tc39 / proposal-temporal

Naming of Temporal.Duration fields #325

615 is feedback on this issue.