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

Jump to bottom

converting -0 to 0 when it appears #2571

Merged
merged 27 commits into from Sep 17, 2021
Merged

converting -0 to 0 when it appears #2571

merged 27 commits into from Sep 17, 2021

Conversation

LucasHFS
Copy link
Contributor

@LucasHFS LucasHFS commented Aug 1, 2021

Attempt to solve #2555 .
handled the possibility of getting result -0 in all uses of Math.ceil and converted it in 0

wenderjean
wenderjean reviewed Aug 2, 2021
View reviewed changes
src/differenceInSeconds/index.ts Outdated
return diff > 0 ? Math.floor(diff) : Math.ceil(diff)
}
return removesNegativeZeroIfPresent(diff > 0 ? Math.floor(diff) : Math.ceil(diff))
}
Copy link

Choose a reason for hiding this comment

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

You a new-line problem here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved!

src/utils/removesNegativeZeroIfPresent/index.ts Outdated
@@ -0,0 +1,3 @@
export default function removesNegativeZeroIfPnumberent(number:number) : number {
return Object.is(number, -0) ? 0 : number
Copy link

Choose a reason for hiding this comment

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

The indentation does not follow the project pattern for those files you created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

src/differenceInHours/index.ts Outdated
@@ -34,5 +35,5 @@ export default function differenceInHours(dirtyDateLeft: Date | number, dirtyDat
const diff =
differenceInMilliseconds(dirtyDateLeft, dirtyDateRight) /
MILLISECONDS_IN_HOUR
return diff > 0 ? Math.floor(diff) : Math.ceil(diff)
return removesNegativeZeroIfPresent(diff > 0 ? Math.floor(diff) : Math.ceil(diff))
Copy link

Choose a reason for hiding this comment

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

The same pattern repeats for all those pieces of code using removesNegativeZeroIfPresent, I mean, diff > 0 ? Math.floor(diff) : Math.ceil(diff). What do u think about moving it to the utility function itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that's a better way to approach it. I did it in the last commit.

@tan75 tan75 linked an issue Aug 2, 2021 that may be closed by this pull request
differenceInSeconds() returns -0 when comparing the same date with millisecond differences #2555
Closed
@pabrodez
Copy link

pabrodez commented Aug 4, 2021 •
edited

After this PR, should date-fns still give the choice of how to round a value? #2511

@tan75
Copy link
Contributor

tan75 commented Aug 5, 2021

After this PR, should date-fns still give the choice of how to round a value? #2511

Good point!
We've discussed this PR internally and have a couple of ideas - I will need to make some changes to these PRs.
The response to your question is 'Yes' - a rounding method will be passed in as an option with Math.floor as a default value.

@tan75
Copy link
Contributor

tan75 commented Aug 12, 2021

@LucasHFS @pabrodez @wenderjean
Just to let you know that we decided to introduce a couple of changes to this PR.

  1. I will delete the utils folder with the getRoundingMethod file - this will be replaced by getRoundingFn from src/_lib.
  2. Object.is is not supported by IE hence we can't use it in the code.
    The latest commit is my attempt to make changes to this PR - I will need to discuss this with @kossnocorp and if this approach is approved, we will be able to go ahead with the changes for the rest of differenceInXX functions.

kossnocorp
kossnocorp requested changes Aug 19, 2021
View reviewed changes
src/_lib/getRoundingFn/index.ts Outdated Show resolved Hide resolved
src/differenceInHours/index.ts Outdated
@@ -1,7 +1,7 @@
import differenceInMilliseconds from '../differenceInMilliseconds/index'
import requiredArgs from '../_lib/requiredArgs/index'

const MILLISECONDS_IN_HOUR = 3600000
import getRoundedValue from '../utils/getRoundedValue'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import getRoundedValue from '../utils/getRoundedValue'
import getRoundedValue from '../utils/getRoundedValue/index'

src/_lib/getRoundingFn/index.ts Outdated Show resolved Hide resolved
fturmel
fturmel reviewed Sep 4, 2021
View reviewed changes
src/differenceInWeeks/test.ts Outdated
const result = differenceInWeeks(
new Date(2014, 6 /* Jul */, 8, 18, 0),
new Date(2014, 5 /* Jun */, 29, 6, 0),
'ceil'
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this argument be either 'trunc' or removed, and the assertion === 1?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fturmel
yes, you are right! my bad!

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ fixed

kossnocorp
kossnocorp requested changes Sep 7, 2021
View reviewed changes
src/_lib/roundingMethods/index.ts Outdated Show resolved Hide resolved
src/_lib/roundingMethods/index.ts Outdated Show resolved Hide resolved
@tan75 tan75 mentioned this pull request Sep 10, 2021
Closed
16 tasks
kossnocorp
kossnocorp approved these changes Sep 16, 2021
View reviewed changes
src/differenceInHours/index.ts Outdated Show resolved Hide resolved
src/differenceInMinutes/index.ts Outdated Show resolved Hide resolved
src/differenceInQuarters/index.ts Outdated Show resolved Hide resolved
src/differenceInSeconds/index.ts Outdated Show resolved Hide resolved
src/differenceInWeeks/index.ts Outdated Show resolved Hide resolved
@kossnocorp kossnocorp merged commit a1384e9 into date-fns:master Sep 17, 2021
@tan75 tan75 mentioned this pull request Oct 2, 2021
Closed
@fturmel fturmel mentioned this pull request Dec 2, 2021
Closed
tan75 added a commit to janziemba/date-fns that referenced this pull request Dec 27, 2021
@LucasHFS @tan75 @kossnocorp
Add roundingMethod option to differenceInX functions (date-fns#2571) (
b884642 
closes date-fns#2555)

Added `roundingMethod` option to `differenceInHours`, `differenceInMinutes`, `differenceInQuarters`, `differenceInSeconds` and `differenceInWeeks` with `trunc` as the default method. 

Co-authored-by: Lucas Silva <lucas.silva@codeminer42.com>
Co-authored-by: Tetiana <ttobin@protonmail.ch>
Co-authored-by: Sasha Koss <koss@nocorp.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kossnocorp kossnocorp kossnocorp approved these changes

@wenderjean wenderjean wenderjean left review comments

@fturmel fturmel fturmel left review comments

@tan75 tan75 tan75 approved these changes

Assignees
No one assigned
Labels
🌟 Improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

differenceInSeconds() returns -0 when comparing the same date with millisecond differences
6 participants
@LucasHFS @pabrodez @tan75 @kossnocorp @wenderjean @fturmel