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

How does plus work? #58

Closed
gibson042 opened this issue Jul 7, 2018 · 11 comments
Closed

How does plus work? #58

gibson042 opened this issue Jul 7, 2018 · 11 comments
Labels
behavior Relating to behavior defined in the proposal

Comments

@gibson042
Copy link
Collaborator

gibson042 commented Jul 7, 2018

From the current README:

Returns a new temporal object with the specified date parts added. Units will be added in order of size, descending.

let myCivilDate = new CivilDate(2016, 2, 29);
let newCivilDate = myCivilDate.plus({years: 1, months: 2});
//results in civil date with value 2017-4-28

Can you add an explanation of this result? Assuming parts are added one-by-one (as opposed to e.g. translating them into nanosecond-counted durations up front, which would be weird because real years and months vary in length), I see at least three possible algorithms:

Option 1: Correct after each part, rounding up

not recommended, can be recovered by chaining calls that don't mix fixed- and variable-length units

  1. Add 1 year, yielding 2017-02-29.
  2. Correct to 2017-03-01.
  3. Add 2 months, yielding 2017-05-01.
  4. (no correction needed)

Option 2: Correct after each part, rounding down

not recommended, can be recovered by chaining calls that don't mix fixed- and variable-length units

  1. Add 1 year, yielding 2017-02-29.
  2. Correct to 2017-02-28.
  3. Add 2 months, yielding 2017-04-28.
  4. (no correction needed)

Option 3 (4): Correct at the end, rounding up (down)

rounding TBD, see #58 (comment)

  1. Add 1 year, yielding 2017-02-29.
  2. Add 2 months, yielding 2017-04-29.
  3. (no correction needed)
@maggiepint
Copy link
Member

This is a good question. I think options 1 and 2 make the most sense to me. Any preferences after up or down? I think we have usually chosen up due to the fact that time only moves forward.

@gibson042
Copy link
Collaborator Author

I think the choice is mostly arbitrary; rounding up is fine. But do note that options 3 and 4 are more general; option 1 or 2 can be built on top of them (e.g., myCivilDate.plus({years: 1}).plus({months: 2})) but the reverse is not true.

@ljharb
Copy link
Member

ljharb commented Jul 10, 2018

That's a decent argument in favor for 3/4, imo

@MrRhodes
Copy link

Just a small thought, can I add a CivilDate and a CivilTime to make a CivilDateTime ?

@RedSquirrelious
Copy link
Contributor

@MrRhodes, do you mean using the .plus function? The CivilTime and CivilDate objects have functions that return CivilDateTime objects (the .withDate and .withTime functions, respectively).

@pipobscure
Copy link
Collaborator

The way I read the original spec (and wrongly so) was to add in increasing order of magnitude:

2016-02-29 + 2m => 2016-04-29
2016-04-29 + 1y => 2017-04-29

which would eliminate rounding entirely. Are we sure we want to add in descending order?

@gilmoreorless
Copy link
Contributor

@pipobscure Adding in increasing order would eliminate rounding for that particular example, but introduce it for other cases:

new CivilDate(2015, 1, 29).plus({years: 1, months: 1});

In decreasing order:
2015-01-29 + 1y => 2016-01-29
2016-01-29 + 1m => 2016-02-29

In increasing order:
2015-01-29 + 1m => 2015-02-29 (invalid date — does this get rounded?)
2015-02-29 + 1y => 2016-02-29

Regardless of whether the parts are added in increasing or decreasing order, there's going to be a case that hits an intermediate rounding problem. My vote is for option 3/4 where the rounding only happens after all parts are added.

@rxaviers
Copy link
Member

rxaviers commented Sep 19, 2018

FWIW, my vote also goes for 3/4. About rounding up or down, picking the "correct" form seems challenging. Follow an observation from the following use cases:

Rounding down:

  • Use case: Scrolling a calendar from month to month, last day of month selected. It would be convenient to have a CivilDate of the last day of the selected month as the initial date and upon scroll, either .plus({months: 1}) (or -1) and the expected result would be the last day of the next (or previous) month
  • Use case: Similar to the one above, but scrolling from year to year. It seems like it would be convenient to "lock" month and day and have year to scroll.

Rounding up:

  • Use case: What day is tomorrow? It's expected for civilDate.plus({days: 1}) to return such result.

My observation so far (from the above use cases)... It seems like it's desirable to always round up the units with lower precision that the one requested (e.g., month when requested is day) and round down the units with higher precision than the one requested (e.g., month when requested is year).

Is there any other use case that breaks this observation?

@jodastephen
Copy link

JSR-310 does the following:

  • combine the years and months to form "totalMonths"
  • add totalMonths, rounding invalid dates (down) to last valid day-of-month
  • add the number of days

This is essentially option 3 above, although expressed in a different way. I still think it is the best choice Options 1 and 2 have nasty effects with the varying month length. I don't believe rounding up is helpful either when doing date time math - if I add 3 months to a date in January I expect to get a date in April, not a date in May..

@pipobscure
Copy link
Collaborator

TL;DR: I don't care what we do, let's just decide and be explicit.


The naive way I did this so far is to round after each step.

  1. Add nanos, micros, millis, seconds, minutes, hours, days, months, years
  2. while (nanos < 0) { micros-=1; nanos += 999; } while (nanos > 999) { micros+=1; nanos-=999 }
  3. while (micros < 0) { millis-=1; micros += 999; } while (micros > 999) { millis+=1; micros-=999 }
  4. while (millis < 0) { seconds-=1; millis += 999; } while (millis > 999) { seconds+=1; millis-=999 }
  5. while (seconds <= 0) { minutes-=1; micros += 60; } while (seconds > 59) { minutes+=1; seconds-=60 }
  6. while (minutes <= 0) { hours-=1; minutes += 60; } while (minutes > 59) { hours+=1; minutes-=60 }
  7. while (hours <= 0) { days-=1; hours += 999; } while (hours > 23) { days+=1; hours-=24 }
  8. while (days < 0) { days += daysInMonth(months); months-=1; } while (days > daysInMonth(months)) { days -= daysInMonth(months); months+= 1; }
  9. while (months < 1) { years-=1; months += 12; } while (months > 12) { years+=1; months-=12 }

CivilDate.fromString('2015-01-29').plus({ years: 1, months: 1 })

2015-01-29 + (1y 1m) => 2016-02-29 (invalid) => 2016-03-01


I'm not saying this is correct, but it's what I did in the polyfill for now. I'm not even sure there is a right way™️ to do this. I just had some conversations with folks over the last few weeks and found that depending on where they are, they have entirely different expectations of the result.

  • There are those that expect +1y +1m to add 13 months an round the day down if the resulting month is shorter. 2016-02-29 + (1y 1m) => 2017-03-29
  • There are those that expect +1y +1m to add 1 year and then round, then add 1 month and round again 2016-02-29 + (1y 1m) => 2017-03-28
  • There are those that expect +1y +1m to add 1 month and then round, then add 1 months and round again 2016-01-31 + (1y 1m) => 2017-02-28

So I think we just need to be absolutely explicit on the rules. This becomes especially relevant when mixing date & time math as they are entirely different animals. Which happens before/after which.

@pipobscure
Copy link
Collaborator

pipobscure commented Oct 27, 2019

I’ve finally worked this out this weekend in the polyfill. I’ve also created a test (or 8070583 tests really) for adding and subtracting all differences for all dates between 1999-01-01 and 2009-12-31.

the properties are
for a earlier than b
let duration = a.difference(b)
duration == b.difference(a)
a.plus(duration) == b
b.minus(duration) == a

additional property is that .difference() always produces durations that in the above operations do not need disambiguation.

@ryzokuken ryzokuken added the behavior Relating to behavior defined in the proposal label Oct 28, 2019
@Ms2ger Ms2ger closed this as completed Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior Relating to behavior defined in the proposal
Projects
None yet
Development

No branches or pull requests