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

fix: Duration add/subtract #2337

Merged
merged 3 commits into from
Jun 24, 2023
Merged

fix: Duration add/subtract #2337

merged 3 commits into from
Jun 24, 2023

Conversation

alexander-ulaev
Copy link
Contributor

fix Duration add/subtract doesn't take into account how many days in a month #2336

@alexander-ulaev alexander-ulaev changed the title fix: Duration add/subtract #2336 fix: Duration add/subtract Jun 15, 2023
@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Merging #2337 (705d9b3) into dev (df49762) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               dev     #2337   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          183       183           
  Lines         2111      2146   +35     
  Branches       555       567   +12     
=========================================
+ Hits          2111      2146   +35     
Impacted Files Coverage Δ
src/plugin/duration/index.js 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

.add(duration.hours(), 'hour')
.add(duration.minutes(), 'minute')
.add(duration.seconds(), 'second')
.add(duration.milliseconds(), 'millisecond')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a little trick to minimize the code, pass the third argument to combine add and subtract together.

// manipulateDuration(this, value, 1)
// manipulateDuration(this, value, -1)

// e.g. .add(duration.months() * (-1), 'month')

also, one thing that confused me, is why adding value.asMilliseconds() is not the correct result.
It should be the real ms between the two dates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the tip to improve the code
value.asMilliseconds() executed without date context and In this case, month is equal to 30 days. I think it's correct. (moment js has same behavior)

.add(duration.hours() * k, 'hour')
.add(duration.minutes() * k, 'minute')
.add(duration.seconds() * k, 'second')
.add(duration.milliseconds() * k, 'millisecond')
Copy link
Owner

@iamkun iamkun Jun 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can replace 'second' with 's' according to https://github.com/iamkun/dayjs/blob/dev/src/utils.js#L31

to save some bytes

the rest LGTM

@iamkun
Copy link
Owner

iamkun commented Jun 24, 2023

Cheers

@iamkun iamkun merged commit 3b1060f into iamkun:dev Jun 24, 2023
github-actions bot pushed a commit that referenced this pull request Jul 1, 2023
## [1.11.9](v1.11.8...v1.11.9) (2023-07-01)

### Bug Fixes

* Add null to min and max plugin return type ([#2355](#2355)) ([62d9042](62d9042))
* check if null passed to objectSupport parser ([#2175](#2175)) ([013968f](013968f))
* dayjs.diff improve performance ([#2244](#2244)) ([33c80e1](33c80e1))
* dayjs(null) throws error, not return dayjs object as invalid date ([#2334](#2334)) ([c79e2f5](c79e2f5))
* objectSupport plugin causes an error when null is passed to dayjs function (closes [#2277](#2277)) ([#2342](#2342)) ([89bf31c](89bf31c))
* Optimize format method ([#2313](#2313)) ([1fe1b1d](1fe1b1d))
* update Duration plugin add/subtract take into account days in month ([#2337](#2337)) ([3b1060f](3b1060f))
* update MinMax plugin 1. ignore the 'null' in args 2. return the only one arg ([#2330](#2330)) ([3c2c6ee](3c2c6ee))
@github-actions
Copy link

github-actions bot commented Jul 1, 2023

🎉 This PR is included in version 1.11.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

BePo65 pushed a commit to BePo65/dayjs that referenced this pull request Sep 20, 2023
## [1.11.9](iamkun/dayjs@v1.11.8...v1.11.9) (2023-07-01)

### Bug Fixes

* Add null to min and max plugin return type ([iamkun#2355](iamkun#2355)) ([62d9042](iamkun@62d9042))
* check if null passed to objectSupport parser ([iamkun#2175](iamkun#2175)) ([013968f](iamkun@013968f))
* dayjs.diff improve performance ([iamkun#2244](iamkun#2244)) ([33c80e1](iamkun@33c80e1))
* dayjs(null) throws error, not return dayjs object as invalid date ([iamkun#2334](iamkun#2334)) ([c79e2f5](iamkun@c79e2f5))
* objectSupport plugin causes an error when null is passed to dayjs function (closes [iamkun#2277](iamkun#2277)) ([iamkun#2342](iamkun#2342)) ([89bf31c](iamkun@89bf31c))
* Optimize format method ([iamkun#2313](iamkun#2313)) ([1fe1b1d](iamkun@1fe1b1d))
* update Duration plugin add/subtract take into account days in month ([iamkun#2337](iamkun#2337)) ([3b1060f](iamkun@3b1060f))
* update MinMax plugin 1. ignore the 'null' in args 2. return the only one arg ([iamkun#2330](iamkun#2330)) ([3c2c6ee](iamkun@3c2c6ee))
BePo65 pushed a commit to BePo65/dayjs that referenced this pull request Sep 23, 2023
## [1.11.9](iamkun/dayjs@v1.11.8...v1.11.9) (2023-07-01)

### Bug Fixes

* Add null to min and max plugin return type ([iamkun#2355](iamkun#2355)) ([62d9042](iamkun@62d9042))
* check if null passed to objectSupport parser ([iamkun#2175](iamkun#2175)) ([013968f](iamkun@013968f))
* dayjs.diff improve performance ([iamkun#2244](iamkun#2244)) ([33c80e1](iamkun@33c80e1))
* dayjs(null) throws error, not return dayjs object as invalid date ([iamkun#2334](iamkun#2334)) ([c79e2f5](iamkun@c79e2f5))
* objectSupport plugin causes an error when null is passed to dayjs function (closes [iamkun#2277](iamkun#2277)) ([iamkun#2342](iamkun#2342)) ([89bf31c](iamkun@89bf31c))
* Optimize format method ([iamkun#2313](iamkun#2313)) ([1fe1b1d](iamkun@1fe1b1d))
* update Duration plugin add/subtract take into account days in month ([iamkun#2337](iamkun#2337)) ([3b1060f](iamkun@3b1060f))
* update MinMax plugin 1. ignore the 'null' in args 2. return the only one arg ([iamkun#2330](iamkun#2330)) ([3c2c6ee](iamkun@3c2c6ee))
ohsory1324 pushed a commit to ohsory1324/dayjs that referenced this pull request Dec 20, 2023
splashwizard pushed a commit to splashwizard/tracking-time that referenced this pull request Oct 21, 2024
## [1.11.9](iamkun/dayjs@v1.11.8...v1.11.9) (2023-07-01)

### Bug Fixes

* Add null to min and max plugin return type ([#2355](iamkun/dayjs#2355)) ([62d9042](iamkun/dayjs@62d9042))
* check if null passed to objectSupport parser ([#2175](iamkun/dayjs#2175)) ([013968f](iamkun/dayjs@013968f))
* dayjs.diff improve performance ([#2244](iamkun/dayjs#2244)) ([33c80e1](iamkun/dayjs@33c80e1))
* dayjs(null) throws error, not return dayjs object as invalid date ([#2334](iamkun/dayjs#2334)) ([c79e2f5](iamkun/dayjs@c79e2f5))
* objectSupport plugin causes an error when null is passed to dayjs function (closes [#2277](iamkun/dayjs#2277)) ([#2342](iamkun/dayjs#2342)) ([89bf31c](iamkun/dayjs@89bf31c))
* Optimize format method ([#2313](iamkun/dayjs#2313)) ([1fe1b1d](iamkun/dayjs@1fe1b1d))
* update Duration plugin add/subtract take into account days in month ([#2337](iamkun/dayjs#2337)) ([3b1060f](iamkun/dayjs@3b1060f))
* update MinMax plugin 1. ignore the 'null' in args 2. return the only one arg ([#2330](iamkun/dayjs#2330)) ([3c2c6ee](iamkun/dayjs@3c2c6ee))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants