Skip to content

Number: Compact formatting with significat digits fix#830

Merged
rxaviers merged 1 commit into
globalizejs:masterfrom
shivijais:significant-digits-rounding
Jun 22, 2018
Merged

Number: Compact formatting with significat digits fix#830
rxaviers merged 1 commit into
globalizejs:masterfrom
shivijais:significant-digits-rounding

Conversation

@shivijais
Copy link
Copy Markdown
Contributor

@shivijais shivijais commented Jun 7, 2018

Fixes: #821

Removed the logic of rounding the number twice - with a precision of {actual precision required + 2} and then again with the actual precision. This was causing wrong rounding off a number. For eg.

Globalize.numberFormatter({
        minimumSignificantDigits: 1,
        maximumSignificantDigits: 3,
    })(12.849872883)

Here the number 12.849872883 is first converted to 12.850 and then when finally rounded off becomes 12.9. But Ideally this should be formatted to 12.8.

@shivijais shivijais force-pushed the significant-digits-rounding branch from d8a9d77 to 559a54e Compare June 7, 2018 06:18
@shivijais
Copy link
Copy Markdown
Contributor Author

@rxaviers Can you please take a look at this PR. The travis-ci is failing on timeout issue with https://github.com/SlexAxton/messageformat which is now moved to https://github.com/messageformat/messageformat

Comment thread test/unit/number/format.js Outdated
minimumSignificantDigits: 1,
maximumSignificantDigits: 3,
compact: "short"
}), "12.8B" );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since these are unit tests, you don't have Globalize available. You need to use format (according to the above tests).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's also add a test where 12850172883 becomes 12.9B

@rxaviers
Copy link
Copy Markdown
Member

rxaviers commented Jun 8, 2018

Thank you so far

@rxaviers
Copy link
Copy Markdown
Member

rxaviers commented Jun 8, 2018

Thanks for the quick fix. Tests are passing, build is failing simply because of commit message which I can fix.

Having said that, I want to be sure the code simplification in the round function doesn't cause any side effect and therefore I plan to review this again. It's on my end, I hope to do it soon and will update this PR.

@shivijais shivijais force-pushed the significant-digits-rounding branch from de61474 to c84da73 Compare June 8, 2018 18:05
@shivijais
Copy link
Copy Markdown
Contributor Author

@rxaviers any update on this?

@rxaviers rxaviers merged commit b08574a into globalizejs:master Jun 22, 2018
@rxaviers
Copy link
Copy Markdown
Member

Thank you! Released as 1.4.0-alpha.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants