fix: signDisplay plus sign behaviour fixed for NumberFormat #1483

shubhamguptadream11 · 2024-08-17T06:33:58Z

Summary

Fixes following issue:

Intl.NumberFormat signDisplay option displays odd behaviour #1466
Android: Intl.NumberFormat style percent not working properly #1138
Intl.NumberFormat on Android with style: 'currency' and signDisplay: 'always' truncates currency sign for positive values #789

Why this change is made?

There are few cases where signDisplay is not being handled correctly.
Examples:

1.
new Intl.NumberFormat('de-DE', {
 style: 'currency',
 currency: 'EUR',
 signDisplay: 'exceptZero',
}).format(8537.71)

Output: +8,537,71+
Expected: +8.537,71 €

2.
new Intl.NumberFormat('ja-JP', {
 style: 'currency', 
 currency: 'JPY', 
 signDisplay: 'exceptZero' 
}).format(123456.789)

Output: +123,457
Expected: +￥123,457

ChangeLog

This PR updates the implementation and testing of the signDisplay functionality in the DecimalFormat class within the Hermes engine, specifically for Android API level 31 and above.

Key Changes:

Implementation:

Integrated the setSignAlwaysShown method of DecimalFormat for API level 31 and above to control the display of the sign (+ or -) based on the signDisplay option. [For more detail about setSignAlwaysShown check here]
For API levels below 31, maintained the existing logic for handling sign display, ensuring backward compatibility.

Test Plan

Added a comprehensive set of test cases in HermesIntlAndroidTest.java to validate the behaviour of the signDisplay functionality, specifically for API level 31 and above.
Test cases cover scenarios for signDisplay: NEVER | ALWAYS | EXCEPTZERO

shubhamguptadream11 · 2024-08-20T03:32:03Z

@tmikov Whenever you have time, can you please check this PR? It solves the signDisplay issue reported many times on the platform.

neildhar · 2024-08-20T17:46:42Z

Hey @shubhamguptadream11, thank you for looking into this and submitting this PR!

Could you describe the exact change here in more detail? It looks like there is more going on with this change, with regards to whether the positive/negative strings are checked and whether things are added to the suffix or prefix.

Alternatively, it looks like Android API level 31 adds native support for this functionality. I think it would be preferable to add an API version check and use the newer API when possible, which should be available on most devices.

… into fix/signDisplay

neildhar · 2024-08-21T16:34:48Z

Looks great!

It looks like CI is failing, likely because the compile/target SDK version specified in our build.gradle is too low. Could you bump that to 31, and add a test for the desired behaviour in HermesIntlAndroidTest.java?

shubhamguptadream11 · 2024-08-22T06:19:46Z

Hi @neildhar,

I attempted to upgrade the compileSdkVersion to 31, but I encountered several issues due to deprecated APIs. For example, InstrumentationTestCase is deprecated as of API level 24 and needs to be replaced with InstrumentationRegistry. Given the scope of these changes, it would be more appropriate to handle this upgrade in a separate PR.

Could someone from the Hermes team take on the task of upgrading the compile/target version to the latest supported? or help me in upgrading that. Once that's done, I'll be able to raise my PR.

Till then I am trying to fix deprecated API's.

Thanks!

shubhamguptadream11 · 2024-08-22T09:50:52Z

Hi @neildhar ,

I've finally fixed all the errors that emerged due to the compile SDK upgrade. Below is a summary of the changes made:

Migration to JUnit4 and AndroidJUnitRunner:

Replaced the deprecated InstrumentationTestCase with the recommended JUnit4 and AndroidJUnitRunner.
Updated the test classes to reflect these changes.

Updated Hermes Tests:

Addressed issues related to missing JSRuntime references by correctly importing and using it within test files.

Please take a moment to review these changes and let me know if any further adjustments are needed.

Thanks for your guidance and support throughout this process!

neildhar · 2024-08-22T15:06:30Z

@shubhamguptadream11, thanks for bumping the SDK version and fixing up the deprecated APIs. Everything looks good, please add a test and update the summary, and I'll merge this.

shubhamguptadream11 · 2024-08-22T16:39:27Z

@neildhar Added test cases and updated PR summary as well. Please check and let me know if anything else needed from my end.
Thanks again!

shubhamguptadream11 · 2024-08-24T04:41:03Z

Hi @neildhar

Just wanted to follow up on this PR. I’ve added the test cases and updated the summary as discussed. Could you please review it when you get a chance? Let me know if there’s anything else you need from my side.

Thanks again for your time!

neildhar · 2024-08-25T19:37:40Z

android/intltest/java/com/facebook/hermes/test/HermesIntlAndroidTest.java

+    if (Build.VERSION.SDK_INT >= 31) {
+      try (JSRuntime rt = JSRuntime.makeHermesRuntime()) {
+        rt.evaluateJavaScript(
+            "var nf = new Intl.NumberFormat('de-DE', { exceptZero: 'never', currency: 'EUR' });\n" +


This test seems odd, is it correct? I'm not aware of exceptZero being an option to NumberFormat (as opposed to signDisplay: "exceptZero"), and I don't believe the currency is printed unless you set style: "currency".

If this is incorrect, we should understand why this test didn't fail.

I've corrected the test case to properly reflect the intended behaviour by using signDisplay: 'exceptZero' and including the style: 'currency' option.

shubhamguptadream11 · 2024-08-28T09:46:51Z

Hi @neildhar ,

Could we please check and review this PR when you have a moment?

neildhar · 2024-08-30T09:48:59Z

@shubhamguptadream11 Could you clarify if the test is actually running or if you ran it locally? It seems odd that the previous version of the test was working.

shubhamguptadream11 · 2024-08-30T11:07:42Z

@neildhar
I am able to run the test cases locally now.

Command Used:

./gradlew :intltest:connectedDebugAndroidTest -Pandroid.testInstrumentationRunnerArguments.class=com.facebook.hermes.test.HermesIntlAndroidTest

With previous faulty test cases

@Test
  public void testSignDisplayExceptZeroForApiLevelAbove31() {
    if (Build.VERSION.SDK_INT >= 31) {
      try (JSRuntime rt = JSRuntime.makeHermesRuntime()) {
        rt.evaluateJavaScript(
            "var nf = new Intl.NumberFormat('de-DE', { exceptZero: 'never', currency: 'EUR' });\n" +
                       "var result = nf.format(8537.71);");

        String result = rt.getGlobalStringProperty("result");
        assertThat(result).isEqualTo("+8.537,71 €");
      }
    }
  }

Test cases are failing in local:

With updated test cases:
Test cases are passing

shubhamguptadream11 · 2024-09-02T05:31:21Z

@neildhar Please check the PR once. I had shared few screenshots above about test cases results.

facebook-github-bot · 2024-09-03T21:42:56Z

@neildhar has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

shubhamguptadream11 · 2024-09-10T12:24:38Z

Hi @neildhar
I hope you're doing well. I wanted to follow up on the status of PR. If there's anything you'd like me to adjust, just let me know.

Thank you for your time and support!

facebook-github-bot · 2024-09-17T07:23:36Z

@shubhamguptadream11 has updated the pull request. You must reimport the pull request before landing.

shubhamguptadream11 · 2024-09-17T07:25:16Z

Hi @neildhar, I hope you are doing well. Can you please take a moment to review this PR.
Thanks!

facebook-github-bot · 2024-09-17T15:50:31Z

@neildhar merged this pull request in a75e14a.

…1483) Summary: Original Author: [email protected] Original Git: a75e14a Original Reviewed By: avp Original Revision: D62153166 Fixes following issue: - #1466 - #1138 - #789 **Why this change is made?** There are few cases where signDisplay is not being handled correctly. Examples: ``` 1. new Intl.NumberFormat('de-DE', { style: 'currency', currency: 'EUR', signDisplay: 'exceptZero', }).format(8537.71) Output: +8,537,71+ Expected: +8.537,71 € 2. new Intl.NumberFormat('ja-JP', { style: 'currency', currency: 'JPY', signDisplay: 'exceptZero' }).format(123456.789) Output: +123,457 Expected: +￥123,457 ``` **ChangeLog** This PR updates the implementation and testing of the signDisplay functionality in the DecimalFormat class within the Hermes engine, specifically for Android API level 31 and above. **Key Changes:** Implementation: - Integrated the **setSignAlwaysShown** method of DecimalFormat for API level 31 and above to control the display of the sign (+ or -) based on the signDisplay option. [For more detail about setSignAlwaysShown check [here](https://developer.android.com/reference/android/icu/text/DecimalFormat#setSignAlwaysShown(boolean))] - For API levels below 31, maintained the existing logic for handling sign display, ensuring backward compatibility. Pull Request resolved: #1483 Pulled By: neildhar Reviewed By: neildhar Differential Revision: D62883944 fbshipit-source-id: df41029b45400b9db038f32727293d5ccda27211

fix: appended plus sign when required

e65d05d

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Aug 17, 2024

shubhamguptadream11 marked this pull request as ready for review August 17, 2024 07:28

Merge branch 'main' into fix/signDisplay

0e80832

shubhamguptadream11 mentioned this pull request Aug 20, 2024

Android: Intl.NumberFormat style percent not working properly #1138

Closed

shubhamguptadream11 mentioned this pull request Aug 20, 2024

Intl.NumberFormat on Android with style: 'currency' and signDisplay: 'always' truncates currency sign for positive values #789

Open

1 task

tmikov assigned neildhar Aug 20, 2024

shubhamguptadream11 and others added 4 commits August 21, 2024 13:24

Merge branch 'main' into fix/signDisplay

0094f7f

fix: added api level 31 check

c3caf4d

Merge branch 'fix/signDisplay' of github.com:dream-sports-labs/hermes…

cc2b679

… into fix/signDisplay

fix: number added

6f8ddbc

shubhamguptadream11 added 2 commits August 22, 2024 11:05

fix: compileSDK version upgraded to 31

d0d8b13

fix: compileSDK version upgraded to 31

5dad8a3

shubhamguptadream11 added 3 commits August 22, 2024 13:28

fix: instrumentation test cases fixed

c330f6c

fix: added import for JS runtime

308f9a4

fix: unused imports removed

783f3c9

feat: added test cases for new API support

22afeaa

neildhar reviewed Aug 25, 2024

View reviewed changes

fix: corrected test cases

aed1135

shubhamguptadream11 and others added 2 commits August 30, 2024 20:45

fix: test case fixed

9ce83eb

Merge branch 'main' into fix/signDisplay

5d9cc93

Merge branch 'main' into fix/signDisplay

b6427b4

facebook-github-bot closed this in a75e14a Sep 17, 2024

facebook-github-bot added the Merged label Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: signDisplay plus sign behaviour fixed for NumberFormat #1483

fix: signDisplay plus sign behaviour fixed for NumberFormat #1483

shubhamguptadream11 commented Aug 17, 2024 •

edited

Loading

shubhamguptadream11 commented Aug 20, 2024 •

edited

Loading

neildhar commented Aug 20, 2024

neildhar commented Aug 21, 2024 •

edited

Loading

shubhamguptadream11 commented Aug 22, 2024 •

edited

Loading

shubhamguptadream11 commented Aug 22, 2024 •

edited

Loading

neildhar commented Aug 22, 2024

shubhamguptadream11 commented Aug 22, 2024

shubhamguptadream11 commented Aug 24, 2024

neildhar Aug 25, 2024

shubhamguptadream11 Aug 26, 2024

shubhamguptadream11 commented Aug 28, 2024

neildhar commented Aug 30, 2024

shubhamguptadream11 commented Aug 30, 2024 •

edited

Loading

shubhamguptadream11 commented Sep 2, 2024

facebook-github-bot commented Sep 3, 2024

shubhamguptadream11 commented Sep 10, 2024

facebook-github-bot commented Sep 17, 2024

shubhamguptadream11 commented Sep 17, 2024

facebook-github-bot commented Sep 17, 2024

fix: signDisplay plus sign behaviour fixed for NumberFormat #1483

fix: signDisplay plus sign behaviour fixed for NumberFormat #1483

Conversation

shubhamguptadream11 commented Aug 17, 2024 • edited Loading

Summary

Test Plan

shubhamguptadream11 commented Aug 20, 2024 • edited Loading

neildhar commented Aug 20, 2024

neildhar commented Aug 21, 2024 • edited Loading

shubhamguptadream11 commented Aug 22, 2024 • edited Loading

shubhamguptadream11 commented Aug 22, 2024 • edited Loading

neildhar commented Aug 22, 2024

shubhamguptadream11 commented Aug 22, 2024

shubhamguptadream11 commented Aug 24, 2024

neildhar Aug 25, 2024

Choose a reason for hiding this comment

shubhamguptadream11 Aug 26, 2024

Choose a reason for hiding this comment

shubhamguptadream11 commented Aug 28, 2024

neildhar commented Aug 30, 2024

shubhamguptadream11 commented Aug 30, 2024 • edited Loading

shubhamguptadream11 commented Sep 2, 2024

facebook-github-bot commented Sep 3, 2024

shubhamguptadream11 commented Sep 10, 2024

facebook-github-bot commented Sep 17, 2024

shubhamguptadream11 commented Sep 17, 2024

facebook-github-bot commented Sep 17, 2024

shubhamguptadream11 commented Aug 17, 2024 •

edited

Loading

shubhamguptadream11 commented Aug 20, 2024 •

edited

Loading

neildhar commented Aug 21, 2024 •

edited

Loading

shubhamguptadream11 commented Aug 22, 2024 •

edited

Loading

shubhamguptadream11 commented Aug 22, 2024 •

edited

Loading

shubhamguptadream11 commented Aug 30, 2024 •

edited

Loading