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

feat(NODE-5579): add Decimal128.fromStringWithRounding() static method #621

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

W-A-James
Copy link
Contributor

@W-A-James W-A-James commented Sep 8, 2023

Description

What is changing?

  • Adds Decimal128.fromStringWithRounding static method
  • Convert test/node/decimal128_tests.js to typescript and add tests for fromStringWithRounding
Is there new documentation needed for these changes?
  • Yes, DOCSP ticket will be filed

What is the motivation for this change?

NODE-5594

Release Highlight

Adds new Decimal128.fromStringWithRounding static method

Prior to the merging of #620, Decimal128.fromString performed rounding behaviour by default that rounded numbers with >34 significant digits incorrectly. In addition to this bug, the intention of the Decimal128 type and specification is to provide an interface to a high-precision floating point value, so rounding by default also went against this intention, which is why that behaviour was removed from the method.

We understand that some users may have come to rely on the rounding behaviour, so that has been corrected here and exposed via a new static method, Decimal128.fromStringWithRounding, which should now allow these users to migrate to the v6 of the driver.

Thank you to @hconn-riparian for reporting a related rounding bug and fix in #560 which has been included in this feature.

// pre v5.5
> let d = Decimal128.fromString('127341286781293491234791234667890123')
new Decimal128("1.273412867812934912347912346678901E+35")

// >= v5.5
> let d = Decimal128.fromString('127341286781293491234791234667890123')
Uncaught:
BSONError: "127341286781293491234791234667890123" is not a valid Decimal128 string - inexact rounding
    at invalidErr (./js-bson/lib/bson.cjs:1402:11)
    at Decimal128.fromStringInternal (./js-bson/lib/bson.cjs:1633:25)
    at Decimal128.fromString (./js-bson/lib/bson.cjs:1424:27)

> d = Decimal128.fromStringWithRounding('127341286781293491234791234667890123')
new Decimal128("1.273412867812934912347912346678901E+35")

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@W-A-James W-A-James changed the base branch from main to 5.x September 8, 2023 14:21
@W-A-James W-A-James changed the base branch from 5.x to NODE-5586 September 8, 2023 14:21
@W-A-James W-A-James changed the title fix(NODE-5546): decimal 128 fromString performs inexact rounding (#613) fix(NODE-5579): add Decimal128.fromStringWithRounding() static method Sep 8, 2023
@W-A-James
Copy link
Contributor Author

Blocking on #620

@W-A-James W-A-James marked this pull request as ready for review September 8, 2023 19:38
@W-A-James W-A-James added the Blocked Blocked on other work label Sep 8, 2023
@nbbeeken
Copy link
Contributor

Should this be "feat"?

@W-A-James W-A-James changed the title fix(NODE-5579): add Decimal128.fromStringWithRounding() static method feat(NODE-5579): add Decimal128.fromStringWithRounding() static method Sep 11, 2023
Base automatically changed from NODE-5586 to 5.x September 11, 2023 21:10
@nbbeeken nbbeeken removed the Blocked Blocked on other work label Sep 11, 2023
@nbbeeken nbbeeken self-assigned this Sep 11, 2023
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 11, 2023
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

As expected some conflicts from the "fix" pr need resolving

#617)

Co-authored-by: hconn-riparian <121891959+hconn-riparian@users.noreply.github.com>
@nbbeeken nbbeeken merged commit 70ca4fc into 5.x Sep 12, 2023
3 checks passed
@nbbeeken nbbeeken deleted the NODE-5579 branch September 12, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants