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

Arithmetic: yes or no? #10

Closed
sffc opened this issue May 7, 2020 · 12 comments · Fixed by #97
Closed

Arithmetic: yes or no? #10

sffc opened this issue May 7, 2020 · 12 comments · Fixed by #97
Labels
Milestone

Comments

@sffc
Copy link
Collaborator

sffc commented May 7, 2020

Do we support arithmetic in Intl.DurationFormat? Examples:

const durf = new Intl.DurationFormat("en", {
  largestUnit: "hours",
  smallestUnit: "minutes"
});


// CASE 1: Input unit is smaller than output unit
// Option A, No Arithmetic (truncate): "0 hours, 2 minutes"
// Option B, With Arithmetic (round to nearest): "0 hours, 3 minutes"
// Option C: Throw exception (out of range)
durf.format(Temporal.Duration.from({
  minutes: 2,
  seconds: 45
}));

// CASE 2: Input unit is larger than output unit
// Option A, No Arithmetic (truncate): "6 hours, 0 minutes"
// Option B, With Arithmetic (balance): "30 hours, 0 minutes"
// Option C: Throw exception (out of range)
durf.format(Temporal.Duration.from({
  hours: 6,
  days: 1
}));

// CASE 3: Input unit is in-range but can be balanced
// Option A, No Arithmetic (do nothing): "0 hours, 75 minutes"
// Option B, With Arithmetic (balance): "1 hour, 15 minutes"
durf.format(Temporal.Duration.from({
  minutes: 75
}));

I have been advocating for Option A, since we have Temporal.Duration to help with the arithmetic. I think Intl.DurationFormat should "pass through" the data from Temporal.Duration, and we should make sure that Temporal.Duration supports all the necessary functions to perform the math.

tc39/proposal-temporal#337

@younies
Copy link
Member

younies commented May 8, 2020

I think balancing should be part of Temporal.Duration, but we need shallow comprising such as detecting zero-valued fields.

@littledan
Copy link
Member

I was expecting balancing to be scoped just to Temporal.Duration, with no extra balancing logic in Intl.DurationFormat.

There's a third option, alongside truncation: scope largestUnit/smallestUnit to just be for zero-extending lower and higher, and throw an exception if truncation of something that's not 0 is needed. (Or, alternatively: represent the 0's differently from missing in Temporal.Duration, which could remove the need for largestUnit/smallestUnit entirely.)

cc @ryzokuken @ptomato

@sffc
Copy link
Collaborator Author

sffc commented May 18, 2020

Here are some examples of what code would look like with arithmetic in Temporal.

Difference between two dates

d1 = Temporal.from("2020-05-18");
d2 = Temporal.from("2020-05-22");
dur = d1.difference(d2);
dur.toLocaleString("es")  // "4 días"

Format from options bag

new Intl.DurationFormat("es").format({
  weeks: 5
})  // "5 semanas"

// Equivalent to:
new Intl.DurationFormat("es").format(
  Temporal.from({
    weeks: 5
  })
)  // "5 semanas"

// Equivalent to:
Temporal.from({ weeks: 5 })
  .toLocaleString("es")  // "5 semanas"

Format from options bag with arithmetic

new Intl.DurationFormat("es").format(
  Temporal.from({ minutes: 90 })
    .balance({ largestUnit: "hours" })
)  // "1 hora, 30 minutos"

// Equivalent to:
Temporal.Duration.from({ minutes: 90 })
  .balance({ largestUnit: "hours" })
  .toLocaleString("es")  // "1 hora, 30 minutos"

Format a number of milliseconds

const milliseconds = 987654321;

// Pass through milliseconds
Temporal.Duration.from({ milliseconds })
  .toLocaleString("en")  // "987,654,321 milliseconds"

// Balance to all fields
Temporal.Duration.from({ milliseconds })
  .balance()
  .toLocaleString("en")  // 11 days, 10 hours, 20 minutes, 54 seconds, 321 milliseconds

// Balance and limit at days and hours
Temporal.Duration.from({ milliseconds })
  .balance({ smallestUnit: "hours" })
  .toLocaleString("en")  // 11 days, 10 hours

@littledan
Copy link
Member

In Temporal.Duration-land, we agreed to support weeks as part of the data model, and to have some kind of balancing/rounding method (details tbd). This seems to point to it being possible to omit arithmetic from Intl.DurationFormat. (We still may want it for ergonomic reasons, so I'm not sure if this closes the issue by itself.)

@sffc
Copy link
Collaborator Author

sffc commented May 29, 2020

My vote is to omit arithmetic from Intl.DurationFormat, and we can add it later if need be.

A possible future setting could be something like:

  • fieldHandling: "reject" (default), "balance"

If we throw exceptions when the fields are out of range on the top or bottom (Option C above), then "reject" would be the default for that potential future option.

@ryzokuken
Copy link
Member

@sffc I too agree that we should avoid arithmetic in DurationFormat.

@younies younies added the consensus We reached a consensus in a discussion meeting, through email or the issue discussion label Jun 15, 2020
@younies
Copy link
Member

younies commented Jun 15, 2020

I do agree with @sffc and we already agreed on Shane's option last meeting,

I will mark this issue as closed for now.

Thanks everyone for your contribution.

@sffc
Copy link
Collaborator Author

sffc commented Mar 16, 2021

According to #32, we decided to add arithmetic back in again. Why the change of heart?

@sffc sffc reopened this Mar 16, 2021
@ryzokuken
Copy link
Member

@sffc I was originally of the mind that we should keep arithmetic in round and only truncate in DurationFormat, but IIRC Justin convinced everyone that implicit rounding is a better option and I don't have any non-esoteric use-cases for truncating the output so I was convinced as well.

@ryzokuken ryzokuken removed the consensus We reached a consensus in a discussion meeting, through email or the issue discussion label Apr 5, 2021
@ryzokuken ryzokuken added this to the Stage 3 milestone Apr 5, 2021
@ryzokuken
Copy link
Member

Can we close this and track the fields to smallestUnit/largestUnit/hideZeroedValues transition in the other issues?

@ryzokuken ryzokuken added the units label Apr 8, 2021
@sffc sffc closed this as completed Apr 13, 2021
@sffc
Copy link
Collaborator Author

sffc commented Apr 27, 2022

In the spec, there is still

EDITOR'S NOTE
It's still being actively discussed if balancing should be done in DurationFormat. For more information, check out the relevant github issue.

Can the spec be updated please, now that the proposal is at Stage 3?

https://tc39.es/proposal-intl-duration-format/#sec-Intl.DurationFormat.prototype.format

@sffc
Copy link
Collaborator Author

sffc commented May 19, 2022

Re-reading #32 and the notes from June 2021 (https://github.com/tc39/ecma402/blob/master/meetings/notes-2021-06-03.md#options-for-smallestunitlargestunit-and-hidezerovalued), and looking at the timestamps, it appears that we did indeed land on having no arithmetic. We should therefore remove the "EDITOR'S NOTE".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants