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

Support optional rounding #170

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

CasperEngl
Copy link

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
Author

Fixes #124 and #142

@CasperEngl
Copy link
Author

@leerob Is anyone going through PRs on this repo?

@leerob
Copy link
Member

leerob commented Dec 1, 2021

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
Author

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

@leerob
Copy link
Member

leerob commented Dec 2, 2021

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
Author

I see. Thank you.

Copy link

@KeithBrown39423 KeithBrown39423 left a comment

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.

Copy link

@plantRhyley plantRhyley left a comment

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

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.

None yet

4 participants