Suggestion: split option name into disambiguation (for DST) vs. overflow (for ranges)

[justingrant]

Of all the concepts in Temporal, the one that confused me most was the disambiguation option.

I finally understood today why this confused me so much: there are two somewhat-unrelated questions mapped to one name.

• How to handle overflow of units like {minute: 66}? Choices are: 'constrain' | 'balance' | 'reject' (or sometimes just 'constrain' | 'reject')
• How to handle DST ambiguity? Choices are 'earlier' | 'later' | 'reject'.

Did I get this correct? If yes, to remove "disambiguation ambiguity" 😃 my suggestion would be to rename the former usage to overflow which is easier to type and spell and would more clearly explain what the option is doing.

An additional benefit would be avoiding potential future conflict. For example, if we build ZonedAbsolute, then its with method would probably need to offer both overflow and DST-ambiguity handling. Even if we don't want to add such a type now, it's probably unwise to preclude it in the future.

[ptomato]

I wouldn't be opposed to changing the options keyword to overflow for the second sense.

(When these were originally separate arguments as opposed to options keys, it wasn't so important what they were called since you could call them whatever you wanted in your code.)

[ryzokuken]

I'm personally in favor of disambiguation. While it's a slightly uncommon word, it means exactly what the parameter does (help Temporal disambiguate between multiple conflicting values) while being general enough to be used across the proposal (as opposed to overflow which is very specific and only applies for a subset of cases).

[justingrant]

Yep, after thinking this over I think the biggest issue with the current naming is potential for future conflict. The particular use case I'm concerned about is a method that converts a {DateTimeLike, TimeZone} pair into an Absolute.

This method would need to have an option to control overflow when converting the DateTimeLike into a valid DateTime AND an option to control DST disambiguation when converting the {DateTime, TimeZone} to Absolute.

If those two options had different names then it'd be easy to combine both options into a single bag, e.g. {disambiguation: 'earlier', overflow: 'reject'}. If the options had the same name, then what would that method's options look like?

The best I've been able to come up with are {disambiguation: {timeZoneOffsetTransition: 'earlier', overflow: 'reject'} } or {timeZoneOffsetTransitionDisambiguation: 'earlier', overflowDisambiguation: 'reject'}. But those seem unnecessarily complex.

Even if Temporal opts not to offer a ZonedAbsolute type (whose from and with methods would need both options), it's likely that someone will want to build an OSS wrapper library that includes a {DateTimeLike, TimeZone} => Absolute method. In either Temporal or OSS, it'd seem better if that method could share the same options types as other methods in Temporal.

[rbuckton]

I was considering something like: { overflow: { disambiguation: 'reject' }, timeZone: { disambiguation: 'earlier' } } for APIs where they are combined. Its somewhat easier to pass around in user code as well:

function f(dateTimeLike, timeZone, options = {}) {
  const dateTime = DateTime.from(dateTimeLike, options.overflow);
  const absolute = dateTime.inTimeZone(timeZone, options.timeZone);
  ...
}

[justingrant]

@rbuckton I was considering something like: { overflow: { disambiguation: 'reject' }, timeZone: { disambiguation: 'earlier' } } for APIs where they are combined.

Yep, that's an OK workaround for userland libraries' internal use, but if options are exposed to users (either in a future Temporal type or in a userland library) then it's still a lot more ergonomic to use different property names. For example, using different property names is over 70% more typing:

{ overflow: { disambiguation: 'reject' }, timeZone: { disambiguation: 'earlier' } }
// vs. 
{ overflow: 'reject' , disambiguation: 'earlier' }

Seems unwise to restrict Temporal to the +70% syntax for any future additions that may require multiple options, or to force userland libraries to choose between ergonomics vs. matching Temporal's options syntax.

Also, the philosophy of Temporal is to use strong typing to avoid ambiguity, so it's unexpected that we're not following the same strong-typed path with options types.

Finally, now that disambiguation: 'balance' is gone from all non-Duration types' from and with options, then the from and with option isn't really doing any "disambiguation" any more. It's simply deciding if invalid values should throw or clamp.

For these reasons, I still encourage renaming this option to overflow.

[ptomato]

I think now that we have decided to add LocalDateTime, this becomes necessary?

[ryzokuken]

Using overflow and disambiguation seems to be a good choice as outlined by @justingrant already.

[justingrant]

In 2020-08-13 meeting, this change was approved. Specifically:

• Usage of disambiguation for DST will be retained as-is
• All other usage of disambiguation will be renamed to overflow.

@ptomato - this will be a straightforward to PR but massive: ~100 files, 1500+ replacements. I'm happy to PR it but it will probably have many merge conflicts with your Negative Durations PR #811. Should we wait to PR this overflow change until your PR is merged, or is it better to make this overflow change first and adapt #811 to use the new overflow name before it's merged?

[ptomato]

I don't mind rebasing #811, but I would hope that the it's close enough to merging now anyway that it wouldn't be an issue.

[justingrant]

Agreed. Let's merge #811 first. Once #811 is merged I'll prepare a PR for this big rename.

[ptomato]

@justingrant #811 is merged now, so this can start. Happy to leave the PR to you, or I can take this one if you're busy.

[justingrant]

@ptomato - If you could take this one that'd be great. My kids just started school and my free time is looking like it's going to be limited over the next few weeks between tech support, homework support, and emotional support for the joy that's remote schooling. ;-) Also this will give me more bandwidth to wrap up remaining LocalDateTime issues (esp. the algorithm testing with the JSCalendar folks). Thanks!

Here's some minor help: the TS declaration from #700 for the new option. I assume we'll want a separate DurationOverflowOptions too.

export interface OverflowOptions {
  /**
   * How to deal with out-of-range values
   *
   * - In `'constrain'` mode, out-of-range values are clamped to the nearest
   *   in-range value.
   * - In `'reject mode'`, out-of-range values will cause the function to throw
   *   a RangeError.
   *
   * The default is `'constrain'`.
   */
  overflow: 'constrain' | 'reject';
  // Assuming name change from `disambiguation` to `overflow`. See #607.
}