Skip to content

Support optional rounding#170

Open
CasperEngl wants to merge 5 commits into
vercel:mainfrom
CasperEngl:feature/optional-rounding
Open

Support optional rounding#170
CasperEngl wants to merge 5 commits into
vercel:mainfrom
CasperEngl:feature/optional-rounding

Conversation

@CasperEngl

Copy link
Copy Markdown

This PR enables rounding through the roundoption.

round defaults to true and current solutions will continue to behave the same.

When round is set to false, all rounding is disabled.

When round is set to a number, it is used as precision for toFixed.

@CasperEngl

Copy link
Copy Markdown
Author

Fixes #124 and #142

@CasperEngl

Copy link
Copy Markdown
Author

@leerob Is anyone going through PRs on this repo?

@leerob

leerob commented Dec 1, 2021

Copy link
Copy Markdown
Contributor

Yes, our current focus right now with ms is stability. We have a canary release that publishes ESM and converts to TypeScript: https://github.com/vercel/ms/releases

@CasperEngl

Copy link
Copy Markdown
Author

All right. I'll see if I can change the target branch from master to canary 👍

@leerob

leerob commented Dec 2, 2021

Copy link
Copy Markdown
Contributor

Those changes have been merged into master (will be main soon), there's just a canary release to test it out first. Main point was we're targeting stability, so likely will be slower to add new features to ms. There are over 100M weekly downloads of ms, so we're extremely cautious about what we add.

Thumbs up on this PR could be a good way to indicate how many people would like to see this feature land 👍

@CasperEngl

Copy link
Copy Markdown
Author

I see. Thank you.

@KeithBrown39423 KeithBrown39423 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

After reviewing this code, I have found nothing wrong with it. It provides the ability to round using any amount and shouldn't cause any problems. I have tested this code multiple times with no error and feel there is no reason the commit not be merged to the main branch.

@maddlesp maddlesp left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This works

I had renamed the package to deploy it to my npm account. This should
not be changed in the upstream.
Deleted since repository uses pnpm
@CasperEngl

Copy link
Copy Markdown
Author

Current conflicts have been resolved. I've also fixed some issues, having to do with the package name and lock file, since I had been using this package myself, deployed to npm.

@sergey-shablenko

sergey-shablenko commented Jul 18, 2025

Copy link
Copy Markdown

are there any updates on this? 4 years old PR with 100 lines is insane

@CasperEngl

Copy link
Copy Markdown
Author

The last update to the package was 4 years ago as well. @leerob do you know who maintains this package nowadays?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants