Repository: moment/luxon (js)

Issue:
DateTime.plus(...) with mixed algebraic signs works incorrectly
## Description
If doing a calculation on `DateTime` with mixed algebraic signs leads to incorrect results. This is true for example when modifying dates in a way that the target month does not have 30 days or the target year is a leap-year.

## Example
```
// should be 2019-12-09, but is 2019-12-10
DateTime.fromISO('2020-01-08').plus({months: -1, days: 1})
```

## Analysis
Internally, the duration `{months: -1, days: 1}` is shifted to `months, days, milliseconds` and during that it is normalized. During normalization, the algebraic signs are changed and the resulting duration is `{months: 0, days: -29}`. It would be correct if the target month has 30 days, but December has 31 days.

Same issue for leap-years: `{years: -4, days: 1}` is converted to `{years: -3, days: -364}`, should be `days: -365` if the resulting year is a leap year.

## Reproducible example
https://codesandbox.io/s/sleepy-diffie-x761e

## Version
Tested with latest Luxon version (1.21.3)

## Workaround
Add units one by one in the right order: `DateTime.fromISO('2020-01-08').plus({months: -1}).plus({days: 1})`

Pull request:
Fix for negative values in plus/minus. Fixes #645 and #669
Mixed positive and negative duration units values are merged when using `shiftTo`, resulting in unintuitive computations.
 
Removing the shift to milliseconds solves this problem, but then breaks computation with fractional values.

Applying the integer part on large units and convert the fractional part to milliseconds solves this issue.

Patch:
diff --git a/docs/math.md b/docs/math.md
index d3815a7e..cc8a98a5 100644
--- a/docs/math.md
+++ b/docs/math.md
@@ -98,10 +98,11 @@ DateTime implements `#valueOf` to return the epoch timestamp, so you can compare
 d1 < d2 // is d1 before d2?
 ```
 
-However, `===` compares object identity (not a useful concept in a library with immutable types) and `#equals` compares both the time and additional metadata, such as the locale and time zone. If you're only interested in checking the equality of the timestamps, you need to coerce them:
+However, be aware that `===` compares object identity, which is not a useful concept in a library with immutable types. Use `#equals` to compare both the time and additional metadata, such as the locale and time zone. If you're only interested in checking the equality of the timestamps, you can use:
 
 ```js
-+d1 === +d2 // are d1 and d2 the same instant in time?
+d1.toMillis() === d2.toMillis() // are d1 and d2 the same instant in time?
++d1 === +d2 // same test, using object coercion
 ```
 
 You may also use `#hasSame` to make more subtle comparisons:
diff --git a/src/datetime.js b/src/datetime.js
index 27089bf7..01532ff5 100644
--- a/src/datetime.js
+++ b/src/datetime.js
@@ -122,22 +122,23 @@ function objToTS(obj, offset, zone) {
 
 // create a new DT instance by adding a duration, adjusting for DSTs
 function adjustTime(inst, dur) {
-  const keys = Object.keys(dur.values);
-  if (keys.indexOf("milliseconds") === -1) {
-    keys.push("milliseconds");
-  }
-
-  dur = dur.shiftTo(...keys);
-
   const oPre = inst.o,
-    year = inst.c.year + dur.years,
-    month = inst.c.month + dur.months + dur.quarters * 3,
+    year = inst.c.year + Math.trunc(dur.years),
+    month = inst.c.month + Math.trunc(dur.months) + Math.trunc(dur.quarters) * 3,
     c = Object.assign({}, inst.c, {
       year,
       month,
-      day: Math.min(inst.c.day, daysInMonth(year, month)) + dur.days + dur.weeks * 7
+      day:
+        Math.min(inst.c.day, daysInMonth(year, month)) +
+        Math.trunc(dur.days) +
+        Math.trunc(dur.weeks) * 7
     }),
     millisToAdd = Duration.fromObject({
+      years: dur.years - Math.trunc(dur.years),
+      quarters: dur.quarters - Math.trunc(dur.quarters),
+      months: dur.months - Math.trunc(dur.months),
+      weeks: dur.weeks - Math.trunc(dur.weeks),
+      days: dur.days - Math.trunc(dur.days),
       hours: dur.hours,
       minutes: dur.minutes,
       seconds: dur.seconds,


Test patch:
diff --git a/test/datetime/math.test.js b/test/datetime/math.test.js
index 381e6227..8c84bfc0 100644
--- a/test/datetime/math.test.js
+++ b/test/datetime/math.test.js
@@ -69,6 +69,16 @@ test("DateTime#plus({ hours: 24 }) gains an hour to spring forward", () => {
   expect(later.hour).toBe(11);
 });
 
+// #669
+test("DateTime#plus({ days:0, hours: 24 }) gains an hour to spring forward", () => {
+  const i = DateTime.fromISO("2016-03-12T10:00", {
+      zone: "America/Los_Angeles"
+    }),
+    later = i.plus({ days: 0, hours: 24 });
+  expect(later.day).toBe(13);
+  expect(later.hour).toBe(11);
+});
+
 test("DateTime#plus(Duration) adds the right amount of time", () => {
   const i = DateTime.fromISO("2016-03-12T10:13"),
     later = i.plus(Duration.fromObject({ day: 1, hour: 3, minute: 28 }));
@@ -106,7 +116,7 @@ test("DateTime#plus renders invalid when out of max. datetime range using second
   expect(d.isValid).toBe(false);
 });
 
-test("DateTime#plus handles franctional days", () => {
+test("DateTime#plus handles fractional days", () => {
   const d = DateTime.fromISO("2016-01-31T10:00");
   expect(d.plus({ days: 0.8 })).toEqual(d.plus({ hours: (24 * 4) / 5 }));
   expect(d.plus({ days: 6.8 })).toEqual(d.plus({ days: 6, hours: (24 * 4) / 5 }));
@@ -115,14 +125,26 @@ test("DateTime#plus handles franctional days", () => {
   );
 });
 
-test("DateTime#plus handles franctional months", () => {
-  const d = DateTime.fromISO("2016-01-31T10:00");
-  expect(d.plus({ months: 8.7 })).toEqual(
-    d.plus({
-      months: 8,
-      milliseconds: Duration.fromObject({ months: 0.7 }).shiftTo("milliseconds")
-    })
-  );
+test("DateTime#plus handles fractional large units", () => {
+  const units = ["weeks", "months", "quarters", "years"];
+
+  for (const unit of units) {
+    const d = DateTime.fromISO("2016-01-31T10:00");
+    expect(d.plus({ [unit]: 8.7 })).toEqual(
+      d.plus({
+        [unit]: 8,
+        milliseconds: Duration.fromObject({ [unit]: 0.7 }).as("milliseconds")
+      })
+    );
+  }
+});
+
+// #645
+test("DateTime#plus supports positive and negative duration units", () => {
+  const d = DateTime.fromISO("2020-01-08T12:34");
+  expect(d.plus({ months: 1, days: -1 })).toEqual(d.plus({ months: 1 }).plus({ days: -1 }));
+  expect(d.plus({ years: 4, days: -1 })).toEqual(d.plus({ years: 4 }).plus({ days: -1 }));
+  expect(d.plus({ years: 0.5, days: -1.5 })).toEqual(d.plus({ years: 0.5 }).plus({ days: -1.5 }));
 });
 
 //------
@@ -183,7 +205,7 @@ test("DateTime#minus renders invalid when out of max. datetime range using secon
   expect(d.isValid).toBe(false);
 });
 
-test("DateTime#minus handles franctional days", () => {
+test("DateTime#minus handles fractional days", () => {
   const d = DateTime.fromISO("2016-01-31T10:00");
   expect(d.minus({ days: 0.8 })).toEqual(d.minus({ hours: (24 * 4) / 5 }));
   expect(d.minus({ days: 6.8 })).toEqual(d.minus({ days: 6, hours: (24 * 4) / 5 }));
@@ -192,13 +214,27 @@ test("DateTime#minus handles franctional days", () => {
   );
 });
 
-test("DateTime#minus handles franctional months", () => {
-  const d = DateTime.fromISO("2016-01-31T10:00");
-  expect(d.minus({ months: 8.7 })).toEqual(
-    d.minus({
-      months: 8,
-      milliseconds: Duration.fromObject({ months: 0.7 }).shiftTo("milliseconds")
-    })
+test("DateTime#minus handles fractional large units", () => {
+  const units = ["weeks", "months", "quarters", "years"];
+
+  for (const unit of units) {
+    const d = DateTime.fromISO("2016-01-31T10:00");
+    expect(d.minus({ [unit]: 8.7 })).toEqual(
+      d.minus({
+        [unit]: 8,
+        milliseconds: Duration.fromObject({ [unit]: 0.7 }).as("milliseconds")
+      })
+    );
+  }
+});
+
+// #645
+test("DateTime#minus supports positive and negative duration units", () => {
+  const d = DateTime.fromISO("2020-01-08T12:34");
+  expect(d.minus({ months: 1, days: -1 })).toEqual(d.minus({ months: 1 }).minus({ days: -1 }));
+  expect(d.minus({ years: 4, days: -1 })).toEqual(d.minus({ years: 4 }).minus({ days: -1 }));
+  expect(d.minus({ years: 0.5, days: -1.5 })).toEqual(
+    d.minus({ years: 0.5 }).minus({ days: -1.5 })
   );
 });