Skip to content

fix: multiply percentages by 100 internally to match FormatJS#44

Merged
ramsey merged 2 commits intomainfrom
fix/percent-calculations
Feb 4, 2022
Merged

fix: multiply percentages by 100 internally to match FormatJS#44
ramsey merged 2 commits intomainfrom
fix/percent-calculations

Conversation

@ramsey
Copy link
Copy Markdown
Contributor

@ramsey ramsey commented Feb 4, 2022

If the parameter is a percent-style number, then we multiply the value
by 100. This is in keeping with the ECMA-402 draft, which specifies the
Intl.NumberFormat rules. When using Intl.NumberFormat to format
percentages, the number must first be multiplied by 100 before any
formatting occurs. See section 15.1.6 of ECMA-402, specifically step
5.b.1

ECMA-402, however, doesn't define an API for MessageFormat, so FormatJS
implements this on their own, using Intl.NumberFormat to process any
number parameters it encounters.2 As a result, all number parameters
in ICU message syntax that specify the ::percent stem (i.e., "{0,
number, ::percent}") have their values first multiplied by 100 before
formatting them.

This may not be considered a bug in FormatJS, since it is adhering to
the ECMA-402 specification. However, it does not follow the rules for
percentages as programmed in icu4c (the underlying library PHP uses),
so in order to match the expected output of FormatJS, we multiply
percent values by 100 before formatting them.

Oddly enough, PHP's NumberFormatter has the same rules, and it uses
the underlying ICU implementation of the number formatter:

$nf = new NumberFormatter('en-US', NumberFormatter::PERCENT);
echo $nf->format(25); // Produces "2,500%"

While:

$mf = new MessageFormatter('en-US', '{0, number, ::percent}');
echo $mf->format([25]); // Produces "25%"

So, one could argue this is a bug in the ICU implementation of the
percent number skeleton.

Product requirements and context

How has this been tested?

PR Checklist

  • I have added tests to cover my changes.

Footnotes

  1. https://tc39.es/ecma402/#sec-partitionnumberpattern

  2. https://formatjs.io/docs/core-concepts/icu-syntax/#number-type

@ramsey ramsey requested a review from a team February 4, 2022 20:20
Comment thread src/Intl/MessageFormat.php
@ramsey ramsey requested review from jmauerhan and xiian February 4, 2022 20:22
if ($numberElement->style->parsedOptions->style === NumberFormatOptions::STYLE_PERCENT) {
$key = $numberElement->value;
if (is_numeric($values[$key])) {
$values[$key] *= 100;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💯

@qlty-cloud-legacy
Copy link
Copy Markdown

Code Climate has analyzed commit c847373 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 97.3% (0.0% change).

View more on Code Climate.

@ramsey ramsey merged commit 4255dfd into main Feb 4, 2022
@ramsey ramsey deleted the fix/percent-calculations branch February 4, 2022 20:54
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.

2 participants