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

roundToNearestMinutes fixes and improvements #3132

Conversation

fturmel
Copy link
Member

@fturmel fturmel commented Jul 26, 2022

Closes #3129

The recent addition of the roundingMethod option (#3091) was not implemented correctly, and some unit tests were not checking for the right behavior. This PR fixes the problems, adds several unit tests, improves the JS docs and adds two additional examples.

I've also internally renamed the getRoundingMethod file and made the function as a default export for consistency to match other functions in the library.

@fturmel
Copy link
Member Author

fturmel commented Jul 26, 2022

@im28 Would you mind reviewing the PR, especially the unit tests to make sure everything checks out and there are no missing edge cases?

@fturmel fturmel requested review from tan75 and leshakoss July 26, 2022 03:51
Copy link

@im28 im28 left a comment

Choose a reason for hiding this comment

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

Looks good to me, Thanks a lot!

@fturmel
Copy link
Member Author

fturmel commented Jul 26, 2022

Just pushed an update to cover an edge case with ceil and milliseconds.

@MrSuttonmann
Copy link

MrSuttonmann commented Aug 5, 2022

Hi, would someone be able to fix these merge conflicts so this can be released? I'm relying on this fix for a development! Thanks :)

@fturmel

@fturmel
Copy link
Member Author

fturmel commented Aug 5, 2022

It seems like main has become the working branch for pre-released v3 since I opened this, which explains all the conflicts.

@leshakoss @tan75 I'm going to wait for you to chime in before changing anything. If you want to accept the PR, let me know if you want to merge into the v2 branch instead, or if I should get rid of the conflicts against main.

Obviously the fixes in this PR should find their way to both v2 and upcoming v3.

@Miyagee
Copy link

Miyagee commented Sep 27, 2022

Any updates on this PR?

@grifotv
Copy link

grifotv commented Oct 12, 2022

While this PR hasn't been merged, I'm using this simple custom solution https://stackoverflow.com/a/10789415

@yannbolliger
Copy link

Would also appreciate this being merged.

@kossnocorp kossnocorp force-pushed the PR/fix-roundToNearestMinutes-rounding-methods branch from 337155f to 92e7b3e Compare January 9, 2024 03:33
@kossnocorp kossnocorp merged commit b15fdac into date-fns:main Jan 9, 2024
6 checks passed
@kossnocorp
Copy link
Member

Shipped with https://github.com/date-fns/date-fns/releases/tag/v3.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Shipped
Development

Successfully merging this pull request may close these issues.

roundToNearestMinutes roundingMethod producing unexpected results
7 participants