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(core): Fix decimal pipe floating point formatting bug #53730

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

Conversation

injae-kim
Copy link

@injae-kim injae-kim commented Dec 30, 2023

PR Type

  • Bugfix

What is the current behavior?

Input (string) : "123456789.123456789"
Format : 0.0-9
Output :   123,456,789.12345679 // 7(8)9 -> 8 is lost..
Expected : 123,456,789.123456789

const num = strToNumber(value);
return formatNumber(num, locale, digitsInfo);

What is the new behavior?

        const formatStrs = [integerPart, fractionPart]
            .map(part => strToNumber(part))
            .map(num => formatNumber(num, locale, digitsInfo));
  • To format string to number on javascript well, apply formatting on integer/fraction part separately and then concat, return it.

Does this PR introduce a breaking change?

  • No

Copy link

google-cla bot commented Dec 30, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@injae-kim injae-kim force-pushed the 47809-fix-decimal-pipe-floating-point branch 3 times, most recently from 75c4a1c to b978193 Compare December 30, 2023 15:49
@injae-kim injae-kim force-pushed the 47809-fix-decimal-pipe-floating-point branch from b978193 to 9d5d49b Compare December 30, 2023 18:08
// e.g. '123456789.123456789'
if (typeof value === 'string' && value.includes('.')) {
const parts = value.split('.');
if (parts.length != 2) {
Copy link
Member

Choose a reason for hiding this comment

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

The condition doesn't cover every non valid string, we should likely rely on strToNumber to throw.

Copy link
Author

@injae-kim injae-kim Dec 30, 2023

Choose a reason for hiding this comment

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

      if (typeof value === 'string' && value.includes('.') && strToNumber(value)) {
        const parts = value.split('.');
        const integerPart = parts[0];
        const fractionPart = `0.${parts[1]}`;
        ..

Hmm you mean calling strToNumber(value) inside of if(..)?
Or can you give some example code? 🙇

Cause parts is array of string, we can't pass it to strToNumber(string) directly 🤔

Copy link
Member

@JeanMeche JeanMeche Dec 30, 2023

Choose a reason for hiding this comment

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

We can either call strToNumber() or probably better, extract the portion we'd like to reuse in a function :

function isStringNumber(value: number|string): boolean {
  return typeof value === 'string' && !isNaN(Number(value) - parseFloat(value))
}

Copy link
Author

Choose a reason for hiding this comment

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

aha~ I understood. isStringNumber looks much better! thanks for nice suggestion 👍

@injae-kim injae-kim force-pushed the 47809-fix-decimal-pipe-floating-point branch 2 times, most recently from edc5ec6 to 940e8d5 Compare December 30, 2023 18:54
fix(core): Fix decimal pipe floating point formatting bug
Address `JeanMeche`'s comments
Extract duplicated codes to isStringNumber()
@injae-kim injae-kim force-pushed the 47809-fix-decimal-pipe-floating-point branch from 940e8d5 to 92d36d0 Compare January 1, 2024 01:54
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Jan 3, 2024
@ngbot ngbot bot modified the milestone: Backlog Jan 3, 2024
@injae-kim
Copy link
Author

I need this feature and ready to update PR, so waiting your awesome reviews~ thanks! 😃

@injae-kim injae-kim requested a review from JeanMeche March 4, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants