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

converting -0 to 0 when it appears #2571

Merged
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

return diff > 0 ? Math.floor(diff) : Math.ceil(diff)
}
return removesNegativeZeroIfPresent(diff > 0 ? Math.floor(diff) : Math.ceil(diff))
}

You a new-line problem here.

Copy link
Contributor Author

@LucasHFS LucasHFS Aug 2, 2021

solved!

@@ -0,0 +1,3 @@
export default function removesNegativeZeroIfPnumberent(number:number) : number {
return Object.is(number, -0) ? 0 : number

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

Copy link
Contributor Author

@LucasHFS LucasHFS Aug 2, 2021

fixed

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

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

@LucasHFS LucasHFS Aug 2, 2021

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

@pabrodez
Copy link

@pabrodez pabrodez commented Aug 4, 2021

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

@tan75
Copy link
Contributor

@tan75 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 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.

src/_lib/getRoundingFn/index.ts Outdated Show resolved Hide resolved
@@ -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

@kossnocorp kossnocorp Aug 19, 2021

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

src/_lib/getRoundingFn/index.ts Outdated Show resolved Hide resolved
const result = differenceInWeeks(
new Date(2014, 6 /* Jul */, 8, 18, 0),
new Date(2014, 5 /* Jun */, 29, 6, 0),
'ceil'
Copy link
Contributor

@fturmel fturmel Sep 4, 2021

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

Copy link
Contributor

@tan75 tan75 Sep 5, 2021

@fturmel
yes, you are right! my bad!

Copy link
Contributor

@tan75 tan75 Sep 5, 2021

fixed

@tan75 tan75 requested review from tan75 and kossnocorp Sep 5, 2021
src/_lib/roundingMethods/index.ts Outdated Show resolved Hide resolved
src/_lib/roundingMethods/index.ts Outdated Show resolved Hide resolved
@tan75 tan75 force-pushed the 2555_negative_zero_result branch from f4fbd2d to 88bc1e4 Sep 8, 2021
@tan75 tan75 mentioned this pull request Sep 10, 2021
16 tasks
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
src/differenceInMinutes/index.ts Outdated Show resolved Hide resolved
src/differenceInSeconds/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
src/differenceInHours/index.ts Outdated Show resolved Hide resolved
src/differenceInHours/index.ts Outdated Show resolved Hide resolved
tan75
tan75 approved these changes Sep 16, 2021
@kossnocorp kossnocorp merged commit a1384e9 into date-fns:master Sep 17, 2021
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants