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

DateTime.plus(...) with mixed algebraic signs works incorrectly #645

Closed
PerfectPixel opened this issue Jan 8, 2020 · 1 comment
Closed

Comments

@PerfectPixel
Copy link

PerfectPixel commented Jan 8, 2020

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})

@icambron
Copy link
Member

icambron commented Jan 8, 2020

Yeah, I agree that's a bug. We probably just shouldn't normalize there

GillesDebunne added a commit to GillesDebunne/luxon that referenced this issue May 31, 2020
GillesDebunne added a commit to GillesDebunne/luxon that referenced this issue May 31, 2020
Requires the `fullConversionMatrices` and `noNormalizationInShiftTo` branches
to be merged first.
GillesDebunne added a commit to GillesDebunne/luxon that referenced this issue Jun 7, 2020
Requires the `fullConversionMatrices` and `noNormalizationInShiftTo` branches
to be merged first.
GillesDebunne added a commit to GillesDebunne/luxon that referenced this issue Jun 9, 2020
Requires the `fullConversionMatrices` and `noNormalizationInShiftTo` branches
to be merged first.
icambron added a commit that referenced this issue Sep 28, 2020
* remove console.log call and bump to 1.24.1

* update package-lock.json

* fix Interval.hasSame behaviour for empty intervals (#712)

Fixes #709

* Mark the package as side-effects-free (#713)

Addresses #710

* Add DATE_MED_WITH_WEEKDAY preset (#716)

* All tests assume America/NewYork timezone (#718)

* Add missing values in conversion matrices (#720)

* Remove normalization in ShiftTo (#721)

* RelativeTime test fails on last day of the month (#719)

* Update wording around date equality test (#725)

* Update wording around date equality test

As discussed in #108

* Fix for negative values in plus/minus. Fixes #645 and #669 (#722)

Requires the `fullConversionMatrices` and `noNormalizationInShiftTo` branches
to be merged first.

* Fixed typos in docs where 'month' was used in place of 'weekday' (#732)

* feat: add support for large durations in seconds (#737)

* Bump lodash from 4.17.14 to 4.17.19 (#739)

Bumps [lodash](https://github.com/lodash/lodash) from 4.17.14 to 4.17.19.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.14...4.17.19)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Interval creation supports Settings.throwOnInvalid. Fixes #728 (#761)

* formatOffset handles signed offsets. Fixes #665 (#756)

* Update package-lock.json after npm install (#751)

Co-authored-by: Gilles Debunne <[email protected]>

* Add tests around DateTime.toISO() (#752)

* Round minutes in timezone offset display. Fixes #724 (#755)

* Support arbitrary precision in ISO milliseconds. Fixes #757 (#758)

* Support arbitrary precision in ISO milliseconds. Fixes #757

* Limit regex to 30 digits to prevent DOS

* fromFormat handles non breakable spaces. Fixes #714 (#762)

* Update Why page, remove outdated Relative references. Fixes #679. (#765)

* bump to 1.25.0

* Introduce DateTime.now() (#766)

* Bump codecov from 3.6.5 to 3.7.1 (#744)

Bumps [codecov](https://github.com/codecov/codecov-node) from 3.6.5 to 3.7.1.
- [Release notes](https://github.com/codecov/codecov-node/releases)
- [Commits](codecov/codecov-node@v3.6.5...v3.7.1)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update endOf() documentation with the same units as startOf() (#779)

* UnitFormat was folded into Unified NumberFormat (#785)

As per tc39/ecma402#32 (comment)

Co-authored-by: Isaac Cambron <[email protected]>
Co-authored-by: downace <[email protected]>
Co-authored-by: Ryota Kameoka <[email protected]>
Co-authored-by: saltire <[email protected]>
Co-authored-by: Jon Knowles <[email protected]>
Co-authored-by: Jörg Bayreuther <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Gilles Debunne <[email protected]>
Co-authored-by: Anthon Holmqvist <[email protected]>
Co-authored-by: Bart Enkelaar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants