diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..a03554d --- /dev/null +++ b/PR.md @@ -0,0 +1,79 @@ +# Fix hour() setter clamping to an invalid hour (24) + +## Problem + +Calling `.hour(n)` with `n >= 24` produces an invalid spacetime whose epoch is +`null`, instead of clamping to a valid hour: + +```js +const s = spacetime('2021-03-14 10:00:00', 'UTC') + +s.hour(24).isValid() // false +s.hour(24).epoch // null +s.hour(25).format('iso') // '' (empty - object is invalid) +``` + +This is inconsistent with every other overflow-clamping setter: + +- `.date(40)` clamps to the last day of the month, +- `.month(13)` clamps to December (11), +- `.hour(-1)` clamps to 0. + +Only the upper bound of `.hour()` is broken. + +## Root cause + +`src/methods/set/set.js`, in `hours()`: + +```js +if (n >= 24) { + n = 24 +} else if (n < 0) { + n = 0 +} +``` + +Valid hours are `0-23`. Clamping to `24` yields a value that +`src/methods/set/walk.js` rejects (`hour.valid: (n) => n >= 0 && n < 24`), and +on an invalid unit `walkTo()` sets `s.epoch = null`, invalidating the whole +object. The negative branch is already correct (clamps to the valid `0`); the +`>= 24` branch is off by one and should clamp to `23`. + +## Fix + +```js +if (n >= 24) { + n = 23 +} else if (n < 0) { + n = 0 +} +``` + +`hour(24)`, `hour(25)`, `hour(100)` now all clamp to `23:00` on the same day, +matching the behavior of the other setters. + +## Test + +Added a test to `test/set.test.js` (next to the existing month/date clamp test) +asserting that out-of-range hours clamp to `23`, keep the object valid, keep the +same day, and that negatives still clamp to `0`. + +Counterfactual (revert `n = 23` back to `n = 24`): + +``` +not ok - hour(24) clamps to 23 + operator: equal + expected: 23 + actual: 0 +``` + +(the invalid object reports hour `0`). + +## Gate + +- `npm test` (tape) run against `src` (the default dev target of + `test/lib/index.js`): the new test passes; the full suite shows only 18 + pre-existing DST/timezone failures (`europe/london`, `atlantic/madeira`, + `atlantic/azores`, some date-dependent "dst-now") that are present verbatim + without this change and are unrelated to it. +- `npm run lint` (eslint ./src): exit 0. diff --git a/src/methods/set/set.js b/src/methods/set/set.js index 1c7d087..33e83c8 100644 --- a/src/methods/set/set.js +++ b/src/methods/set/set.js @@ -67,8 +67,10 @@ const minutes = function (s, n, goFwd) { const hours = function (s, n, goFwd) { n = validate(n) + // clamp to a valid hour (0-23). Clamping to 24 produces an hour that + // walkTo() rejects, which nulls the epoch and invalidates the object. if (n >= 24) { - n = 24 + n = 23 } else if (n < 0) { n = 0 } diff --git a/test/set.test.js b/test/set.test.js index 9efdcee..8acbaa2 100644 --- a/test/set.test.js +++ b/test/set.test.js @@ -238,3 +238,17 @@ test('month()/date() setters clamp the day to the month length', (t) => { t.equal(iso(spacetime('2021-02-10').date(31)), '2021-02-28', 'date(31) in a common february clamps to the 28th') t.end() }) + +test('hour() setter clamps out-of-range hours to a valid hour', (t) => { + const s = spacetime('2021-03-14 10:00:00', 'UTC') + // an out-of-range hour must clamp to the last valid hour (23), not produce + // an invalid spacetime with a null epoch + t.equal(s.hour(24).hour(), 23, 'hour(24) clamps to 23') + t.equal(s.hour(25).hour(), 23, 'hour(25) clamps to 23') + t.equal(s.hour(100).hour(), 23, 'hour(100) clamps to 23') + t.equal(s.hour(24).isValid(), true, 'hour(24) stays valid') + t.equal(s.hour(24).format('iso-short'), '2021-03-14', 'hour(24) keeps the same day') + // negative hours still clamp to 0 + t.equal(s.hour(-1).hour(), 0, 'hour(-1) clamps to 0') + t.end() +})