deps: backport Intl.NumberFormat fix#6275
Closed
targos wants to merge 2 commits intonodejs:v4.x-stagingfrom
Closed
deps: backport Intl.NumberFormat fix#6275targos wants to merge 2 commits intonodejs:v4.x-stagingfrom
targos wants to merge 2 commits intonodejs:v4.x-stagingfrom
Conversation
Original commit message:
Make NumberFormat use the ICU currency data, fix bug in NumberFormat
NumberFormat previously just used a min of 0 digits after the decimal and a max of 3. This CL changes it so that we use the ICU currency data, and set the min and max to the number of numbers after the decimal point for each currency.
This CL also fixes a small bug where if the minimum fraction digits is above 3 but the maximum fraction digits isn't set, then it returns with only three numbers after the decimal point.
BUG=435465,473104,304722
LOG=Y
Review URL: https://codereview.chromium.org/1231613006
Cr-Commit-Position: refs/heads/master@{nodejs#29734}
Original commit message:
Fix user options for fractional digits in Intl.NumberFormatter
The patch in https://crrev.com/ddb5c2d999c5ee6e31c4a9599bb3ddb293cc3f49
moved all fractional digit settings to default values due to a coding
error. These were not even correct default values, and users observed
errors where percentages were written as "23.0%" instead of "23%".
This patch fixes the setting propagation when appropriate and it changes
the default max fractional digits of a percentage to 0, per spec.
BUG=chromium:544122
R=mnita,jochen
CC=hichris123,adamk
LOG=Y
Review URL: https://codereview.chromium.org/1420883002
Cr-Commit-Position: refs/heads/master@{nodejs#31468}
targos
added
v8 engine
Member
Author
Member
|
Can't these be cherry-picked upstream into the 5.0 release branch? |
Member
Author
|
They already are in the 5.0 branch (it even goes back to 4.6). This is a backport for LTS only. |
Member
|
Apologies, missed that the target branch was v4.x-staging. LGTM. @mhdawson has been working on making the V8 test suite run on our CI but I'm not sure if v4.x is already included. If it is, it would be good to run this PR through it. |
Member
|
Well, even with the V8 tests it might be worthwhile adding a quick regression test in |
jasnell
pushed a commit
that referenced
this pull request
Apr 20, 2016
Original commit message:
Make NumberFormat use the ICU currency data, fix bug in NumberFormat
NumberFormat previously just used a min of 0 digits after the decimal and a max of 3. This CL changes it so that we use the ICU currency data, and set the min and max to the number of numbers after the decimal point for each currency.
This CL also fixes a small bug where if the minimum fraction digits is above 3 but the maximum fraction digits isn't set, then it returns with only three numbers after the decimal point.
BUG=435465,473104,304722
LOG=Y
Review URL: https://codereview.chromium.org/1231613006
Cr-Commit-Position: refs/heads/master@{#29734}
PR-URL: #6275
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell
pushed a commit
that referenced
this pull request
Apr 20, 2016
Original commit message:
Fix user options for fractional digits in Intl.NumberFormatter
The patch in https://crrev.com/ddb5c2d999c5ee6e31c4a9599bb3ddb293cc3f49
moved all fractional digit settings to default values due to a coding
error. These were not even correct default values, and users observed
errors where percentages were written as "23.0%" instead of "23%".
This patch fixes the setting propagation when appropriate and it changes
the default max fractional digits of a percentage to 0, per spec.
BUG=chromium:544122
R=mnita,jochen
CC=hichris123,adamk
LOG=Y
Review URL: https://codereview.chromium.org/1420883002
Cr-Commit-Position: refs/heads/master@{#31468}
PR-URL: #6275
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of change
Backport two V8 fixes to Intl to fix issues in
Intl.NumberFormat.Fixes: #6260
cc @srl295 @nodejs/lts