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

fix(toDecimal): preserve negative sign for leading zeros #693

Merged
merged 2 commits into from
Dec 19, 2022

Conversation

jparise
Copy link
Contributor

@jparise jparise commented Dec 10, 2022

Change #690 fixed the general case of formatting negative unit values, but was one remaining case where formatting was still incorrect: when the first unit is -0, the resulting string didn't include the leading negative sign.

This change identifies that exact case (negative value, leading zero) and pads the resulting string with a leading negative sign.

Fixes #692

@vercel
Copy link

vercel bot commented Dec 10, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
dinerojs ✅ Ready (Inspect) Visit Preview Dec 18, 2022 at 10:11PM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 10, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4eed6c3:

Sandbox Source
@dinero.js/example-cart-react Configuration
@dinero.js/example-cart-vue Configuration
@dinero.js/example-pricing-react Configuration
@dinero.js/example-starter Configuration

packages/core/src/api/toDecimal.ts Outdated Show resolved Hide resolved
packages/core/src/api/toDecimal.ts Outdated Show resolved Hide resolved
packages/core/src/api/toDecimal.ts Outdated Show resolved Hide resolved
packages/core/src/api/toDecimal.ts Outdated Show resolved Hide resolved
packages/core/src/api/toDecimal.ts Outdated Show resolved Hide resolved
packages/core/src/api/toDecimal.ts Outdated Show resolved Hide resolved
Change dinerojs#690 fixed the general case of formatting negative unit values,
but was one remaining case where formatting was still incorrect: when
the first unit is -0, the resulting string didn't include the leading
negative sign.

This change identifies that exact case (negative value, leading zero)
and pads the resulting string with a leading negative sign.

Fixes dinerojs#692
@jparise
Copy link
Contributor Author

jparise commented Dec 12, 2022

Given that getDecimal always receives a two-element units array, I took a more "drastic" approach and unrolled the .map() operation. We were specializing each element (first, last) within the mapping function anyway.

(getDecimal could assert(units.length === 2, ...), but I think the assert() in toDecimal already covers this "must be decimal" case?)

I think this results in a simpler solution overall that only uses a single top-level branch (to determine if we need to add the leading negative).

@johnhooks
Copy link
Contributor

@jparise it reads very clearly to me.

Copy link
Collaborator

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

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

Excellent rewrite! A few suggestions and we'll be good to go.

packages/core/src/api/toDecimal.ts Outdated Show resolved Hide resolved
packages/core/src/api/toDecimal.ts Outdated Show resolved Hide resolved
packages/core/src/api/toDecimal.ts Outdated Show resolved Hide resolved
packages/core/src/api/toDecimal.ts Show resolved Hide resolved
Co-authored-by: Sarah Dayan <5370675+sarahdayan@users.noreply.github.com>
Copy link
Collaborator

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

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

Awesome contribution, thanks @jparise!

@sarahdayan sarahdayan merged commit e6f290d into dinerojs:main Dec 19, 2022
@jparise jparise deleted the todecimal-negative-zero branch December 19, 2022 15:50
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.

[Bug]: toDecimal fails to print a leading negative sign when the first unit is negative zero
3 participants