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.

Sign up for GitHub

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

Support optional rounding #170

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

Support optional rounding #170

wants to merge 5 commits into 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 👍

@leerob leerob mentioned this pull request Dec 2, 2021
Closed
@CasperEngl
Copy link
Author

I see. Thank you.

KeithBrown39423
KeithBrown39423 approved these changes Jan 8, 2022
View reviewed changes
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.

SomeoneOrderedATransfem
SomeoneOrderedATransfem approved these changes Mar 10, 2022
View reviewed changes
Copy link

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

@CasperEngl
Merge remote-tracking branch 'upstream/master'
0b523a7
@CasperEngl
fix: package name
6a9d2e6 
I had renamed the package to deploy it to my npm account. This should
not be changed in the upstream.
@CasperEngl
chore: delete package-lock
afced27 
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
Reviewers

@KeithBrown39423 KeithBrown39423 KeithBrown39423 approved these changes

@SomeoneOrderedATransfem SomeoneOrderedATransfem SomeoneOrderedATransfem approved these changes

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
4 participants
@CasperEngl @leerob @SomeoneOrderedATransfem @KeithBrown39423