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

Default largestUnit to the largest non-zero unit #980

Closed
ryzokuken opened this issue Oct 12, 2020 · 6 comments
Closed

Default largestUnit to the largest non-zero unit #980

ryzokuken opened this issue Oct 12, 2020 · 6 comments

Comments

@ryzokuken
Copy link
Member

From #978 (comment),

I like this idea of "top-heavy". Would it be correct to say that if largestUnit is missing, it defaults to the largest nonzero unit in the duration? If so, then would we be able to apply a classic, deterministic balance algorithm (convert everything to the smallest unit, then balance up to the largest unit)?

/cc @sffc @justingrant

@justingrant
Copy link
Collaborator

Would it be correct to say that if largestUnit is missing, it defaults to the largest nonzero unit in the duration?

More or less. Here's the planned behavior from #856:

7. We'll add an 'auto' value for largestUnit in Duration and non-Duration difference methods where it's used. 'auto', like undefined, will select the default unit. EDIT 2020-10-06: added this section

  • 6.1 On Duration methods, 'auto' means "the largest nonzero unit in the input(s)"

  • 6.2 On difference of non-Duration types 'auto' means to pick the default unit of the type:

    • 6.2.1 Instant: 'seconds'
    • 6.2.2 Date / DateTime - 'days'
    • 6.2.3 ZonedDateTime - 'hours'
    • 6.2.4 Time - 'hours'
    • 6.2.5 YearMonth - 'years'
  • 6.3 If the default chosen above is smaller than the user-provided smallestUnit then set largestUnit = smallestUnit.

@ryzokuken, does this proposed behavior above match what you had in mind?

If so, then would we be able to apply a classic, deterministic balance algorithm (convert everything to the smallest unit, then balance up to the largest unit)?

Not sure. @ptomato is closer to this problem as he's implementing the changes agreed in #856 (comment), so he'll likely have a more informed opinion. One caveat might be that there's some special-casing around weeks (see #856 (comment)) that probably means that the inputs to a "classic, deterministic balance algorithm" might require a boolean inputHasWeeks parameter to know whether the output should include weeks or not.

@ptomato
Copy link
Collaborator

ptomato commented Oct 28, 2020

What needs to be done in this issue?

@sffc
Copy link
Collaborator

sffc commented Oct 28, 2020

Mainly docs I think, and verify that the spec and polyfill uses this behavior.

@ptomato
Copy link
Collaborator

ptomato commented Oct 28, 2020

I'm not 100% sure what "this behavior" is — does it apply only to Duration methods with a largestUnit parameter?

@justingrant
Copy link
Collaborator

I'm not 100% sure what "this behavior" is — does it apply only to Duration methods with a largestUnit parameter?

AFAIK this is correct: round, add, subtract are AFAIK the only methods affected.

AFAIK, this issue is part of #856 and can be closed, unless it's better to leave this open because #856 includes a bunch of other stuff too. The balancing.md page has already been updated for #856 behavior, and docs for round, add, subtract should be updated when their #856 code changes are made.

ptomato added a commit that referenced this issue Oct 28, 2020
Per #980 it is a recognized operation to get the correct largestUnit value
corresponding with the largest non-zero unit in the duration. Generalize
this operation in the spec, in order to use in #857.
@ptomato
Copy link
Collaborator

ptomato commented Oct 28, 2020

In #1071 I've made a separate abstract operation out of the logic described here, and it's used in Duration's round, add, and subtract methods, so I'll close this.

@ptomato ptomato closed this as completed Oct 28, 2020
Ms2ger pushed a commit that referenced this issue Oct 29, 2020
Per #980 it is a recognized operation to get the correct largestUnit value
corresponding with the largest non-zero unit in the duration. Generalize
this operation in the spec, in order to use in #857.
Ms2ger pushed a commit that referenced this issue Oct 29, 2020
Per #980 it is a recognized operation to get the correct largestUnit value
corresponding with the largest non-zero unit in the duration. Generalize
this operation in the spec, in order to use in #857.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants