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

Suggestion: consistent balancing behavior for Duration's plus and minus methods #645

Closed
justingrant opened this issue Jun 3, 2020 · 6 comments · Fixed by #811
Closed

Comments

@justingrant
Copy link
Collaborator

Duration.prototype.minus supports balancing, but Duration.prototype.plus doesn't. The docs admit that including balance in minus is not strictly required:

The default is balanceConstrain mode. The balance mode is only provided for convenience

Why does only minus get "convenience"? This inconsistency seems unnecessary and will makes things harder for new users. Seems like either both methods should balance, or neither of them should.

One option would be to remove balancing from the options for all Duration methods (including with and from), and add a new balanced method to do the balancing. I think @sffc recommended adding this kind of method in another issue, but I can't find it at the moment.

Another option would be to add a balance option to plus so that all four Duration methods would have consistent behavior.

But I don't think that the current inconsistent state makes sense.

Note that if we provide negative durations (#558), then parallel behavior between plus and minus will be required regardless.

To illustrate the problem, here's a use-case: "adjust a meeting length by a signed number of minutes and return a balanced result"

function adjustByMinutes(duration, minutes) {
  if (minutes < 0) { 
    return duration.minus({minutes}, {disambiguation: 'balance'});
  } else {
    // expected: 
    // return duration.plus({minutes}, {disambiguation: 'balance'});

    // `minus ({}` is the shortest "chained" solution in current polyfill,
    // because `with` will throw if passed an empty object
    duration.plus({minutes}).minus({}, {disambiguation: 'balance'});

    // non-chained alternative
    // Temporal.Duration.from(duration.plus({minutes}), {disambiguation: 'balance'});
  }
}

Another challenge with the current behavior is that balancing for plus results is not discoverable via reading code or via IDE autocomplete.

@sffc
Copy link
Collaborator

sffc commented Jun 3, 2020

I think @sffc recommended adding this kind of method in another issue, but I can't find it at the moment.

#337 (comment)

@justingrant
Copy link
Collaborator Author

Excerpting Shane's idea from #337 (comment):

const d = Temporal.Duration.from({ days: 10, hours: 23 });
const d1 = d.balance({ largestUnit: "weeks", calendar: "iso" });  // 1 week 3 days 23 hours
const d1 = d.balance({ smallestUnit: "days" });  // 11 days (or 10 days?)

If doing math with weeks or higher, you just have to pass in which calendar you want.

Note that balancing from hours<->days like the examples above requires a time zone and starting Absolute if we want to support DST properly. (Can discuss that further in #559 to keep this issue focused on plus).

@ptomato
Copy link
Collaborator

ptomato commented Jun 12, 2020

With unsigned durations, addition is fundamentally a different operation than subtraction. Addition can only overflow to infinity, that is, if we make the change in #664 so that disambiguation doesn't apply to the edge of the representable range, then there's not even any use for disambiguation in Duration.plus. If we keep unsigned durations and the balance/balanceConstrain is a problem, then I'd advocate implementing #664 and dropping disambiguation from Duration arithmetic. (i.e., plus is always reject, minus is always balanceConstrain), and adding the balance method from #337.

With signed durations, then I agree about making the behaviour of plus and minus consistent.

@justingrant
Copy link
Collaborator Author

justingrant commented Jul 14, 2020

Related feedback from @romulocintra in #772:

The value of duration shouldn't be PT24H59S or PT24H1M59S ?

Temporal.Duration.from({ hours: 23, minutes: 59, seconds: 59 }).plus({ minutes : 1}).toString();
//=> PT23H60M59S

Did not had a read on the spec to determine if this is a bug or intentional

I assume he means PT24H59S above. PT24H1M59S would never be right.

@ptomato
Copy link
Collaborator

ptomato commented Aug 25, 2020

I believe this is done in #811, actually?

@ptomato ptomato removed the feedback label Aug 25, 2020
@justingrant
Copy link
Collaborator Author

I believe this is done in #811, actually?

Yep. #811 should fix this issue. Specifically (from #782):

  • 6.4 Because Duration's plus and minus methods can now perform addition or subtraction according to the sign of the duration, both methods must now accept identical options. Currently plus uses 'constrain' (default) and 'reject' while minus uses 'balanceConstrain' (default) and 'balance'. To align these and to retain consistency with other Temporal uses, we'll rename 'balanceConstrain' to 'constrain'. Both methods will now accept 'constrain' (default), 'reject', or 'balance'.

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

Successfully merging a pull request may close this issue.

3 participants