Adding Percent Formatter to Experimental #5255

blaynem · 2024-07-17T20:30:17Z

What that PR doin?

Creating a Percent Formatter! solving #4483

The formatter includes the option to display as an Approximate, Standard, or Explicit Plus values.

Percent Subpatterns

Pre-requisite reading: Number Pattern Character Definitions

There can be two types of subpatterns from CLDR that are split with the ; character. We always get an explicit positive subpattern, some locales include an explicit negative subpattern as well. When there is no explicit negative subpattern, an implicit negative subpattern is formed from the positive pattern with a prefixed - (ASCII U+002D HYPHEN-MINUS).

To give an example, below is a table where a value of 123 is used for the positive pattern, and -123 is used for the negative pattern. Notice the difference between tr and blo locale where the minus sign is appended to the beginning, even before the % sign.

locale	pattern	positive pattern	negative pattern	explicit?
en-US	#,##0%	123%	-123%
fr	#,##0 %	123 %	-123 %
tr	%#,##0	%123	-%123
blo	% #,#0;% -#,#0	% 123	% -123	true

Display Options

There are 3 Display options: Approximate, Standard, and ExplicitPlus. Below are some examples

value	pattern	Standard	Approximate	ExplicitPlus
123	#,##0%	123%	~123%	+123%
123	% -#,#0	% 123	% ~123	% +123
123	% #,#0	% 123	~% 123	+% 123
-456	#,##0%	-456%	~-456%	+-123%
-456	% -#,#0	% -456	% ~-456	% +-456
-456	% #,#0	-% 456	~-% 456	+-% 456

Approximate

If the - symbol placeholder is present in the pattern, then the approximate sign gets prepended to the sign that is being used.
If the - symbol placeholder is not present, then we follow the implicit
It does not do any sort of rounding to the value itself.

Explicit Plus

If the ExplicitPlus display option is chosen, we are simply ensuring that the plus sign is always present. We follow the typical rules of the negative subpattern while ensuring the localized plus sign is included.

Open Discussion

There are a few things left to determine.

If the Approximate display option is selected, should the value be rounded to the nearest whole number?
- It could go either way. I'd vote no to keep the formatter a bit more "pure" to just accept the value it's going to format.
~~If the Explicit Plus display option is selected, should the value be converted to the absolute value?~~
- My assumption is that the end user wants to show a number with the + or - sign, not necessarily for it to be explicitly a plus. If that's the case, maybe this display option should be renamed to AlwaysShowSign instead?

sffc · 2024-07-17T20:35:40Z

components/experimental/src/dimension/percent/format.rs

+        W: core::fmt::Write + ?Sized,
+    {
+        // Construct the percent sign value.
+        let percent_sign_value = format!(


This is close but it won't build because format! is in the prelude only in std, but this should work with no_std.

Since you did your work last winter, we now have the icu_pattern utility which does this stuff better. It will make the smallest, cleanest, fastest code if you change PercentEssentialsV1 to use icu_pattern.

blaynem · 2024-07-18T19:20:25Z

CONTRIBUTING.md

@@ -79,6 +79,7 @@ There are various files that auto-generated across the ICU4X repository.  Here a
 need to run in order to recreate them.  These files may be run in more comprehensive tests such as those included in `cargo make ci-job-test` or `cargo make ci-all`.

 - `cargo make testdata` - regenerates all test data in the `provider/testdata` directory.
+- `cargo make bakeddata experimental` - regenerates baked data in the `provider/data/experimental` directory.


I couldn't find a note of this anywhere. It's definitely my inexperience with Rust and tracking down where commands come from. But felt it might be helpful to have it in here!

…t plus pattern

blaynem · 2024-07-19T23:03:30Z

components/experimental/src/dimension/percent/format.rs

+            // TODO: Determine if we throw an error when explicit plus is called and the value is negative.
+            Display::ExplicitPlus => self
+                .essential
+                .negative_pattern
+                .interpolate([self.value.to_string(), self.essential.plus_sign.to_string()])
+                .write_to(sink)?,
+        };


Unsure what to do for this portion. There's one of 2 assumptions we could make here.

They want an absolute value. Example:
a. Input: -123 -> output +123%

They just want any visible sign (plus or minus).
a. Input: -123 -> output -123%
b. Input: 123 -> output +123%

I think my assumption would be 2.

I'm certain it is 2, the plus is usually implicit, this makes it explicit.

blaynem · 2024-07-19T23:25:40Z

components/experimental/src/dimension/provider/percent.rs

+/// https://www.unicode.org/reports/tr35/tr35-numbers.html#approximate-number-formatting
+/// https://www.unicode.org/reports/tr35/tr35-numbers.html#explicit-plus-signs


Should I keep or remove these 2 links? I think they're super beneficial to glance at, but fine removing them

CONTRIBUTING.md

robertbastian · 2024-07-24T08:49:59Z

components/experimental/src/dimension/percent/format.rs

+                    Sign::Negative => self.value.clone().with_sign(Sign::None),
+                    _ => self.value.to_owned(),
+                }
+                .to_string();


issue: you shouldn't be calling to_string in a Writeable implementation. I believe because FixedDecimal: Writable, you can pass it directly to interpolate if you pass in a tuple instead of an array (that allows the arguments to have different types).

Didn't notice I could do a tuple with different values! I was able to convert that like you said.

Thanks for calling out the intermediate allocations. Coming from a JS background, so it's something that I need to be more thoughtful about.

components/experimental/src/dimension/percent/format.rs

robertbastian · 2024-07-24T09:28:40Z

components/experimental/src/dimension/percent/format.rs

+            // TODO: Determine if we throw an error when explicit plus is called and the value is negative.
+            Display::ExplicitPlus => self
+                .essential
+                .negative_pattern
+                .interpolate([self.value.to_string(), self.essential.plus_sign.to_string()])
+                .write_to(sink)?,
+        };


I'm certain it is 2, the plus is usually implicit, this makes it explicit.

components/experimental/src/dimension/percent/format.rs

robertbastian · 2024-07-24T09:50:37Z

components/experimental/src/dimension/percent/format.rs

+        let result = format_percent(&locale, Default::default(), positive_value);
+        assert_eq!(result.as_str(), "12345.67%");


issue: this should use assert_writeable_eq!

Suggested change

let result = format_percent(&locale, Default::default(), positive_value);

assert_eq!(result.as_str(), "12345.67%");

let default_fmt = PercentFormatter::try_new(&locale, Default::default()).unwrap();

let formatted_percent = default_fmt.format_percent(&positive_value);

assert_writeable_eq!(formatted_percent, "12345.67%");

Was running into an issue there with the Display writeable. Found the macro to use in another formatter!
writeable::impl_display_with_writeable. So those are fixed.

robertbastian · 2024-07-24T09:53:18Z

components/experimental/src/dimension/percent/format.rs

+    }
+
+    #[test]
+    pub fn test_fr_fr() {


issue: these tests are all almost identical, the code can be significantly deduplicated. However, what's the value in having this test essentially 7 times? What code path does test_fr_fr test that test_en_us doesn't? I think one LTR and one RTL language should be sufficient test coverage.

I definitely went overboard. I'll cut this down to a few of them!

components/experimental/src/dimension/percent/formatter.rs

sffc

Nice work overall! One thing you're missing.

sffc · 2024-07-25T02:49:35Z

components/experimental/src/dimension/percent/format.rs

+                if self.value.sign() == Sign::Negative {
+                    self.essential
+                        .negative_pattern
+                        .interpolate((abs_value, &self.essential.minus_sign))


Issue, here and elsewhere: instead of using impl Writeable for FixedDecimal, you should be using impl Writeable for FormattedFixedDecimal, which you can obtain from a FixedDecimalFormatter, which you should include as a field of the PercentFormatter.

Without the FixedDecimalFormatter, you won't get localized digits, decimal separators, or grouping separators.

You can test this with locales such as bn and ar-EG.

Good callout! Added

components/experimental/src/dimension/percent/formatter.rs

robertbastian

This is great work!

I think we can merge it like this, however the next step should be to use FixedDecimalFormatter for correct number formatting.

robertbastian · 2024-07-25T08:18:30Z

components/experimental/src/dimension/provider/percent.rs

+    /// Represents the standard pattern for negative percents.
+    /// NOTE: place holder 0 is the place of the percent value.
+    ///       place holder 1 is the place of the plus, minus, or approximate signs.
+    pub negative_pattern: DoublePlaceholderPattern<Cow<'data, str>>,


nit: maybe this should be signed_pattern and the other one unsigned_pattern, because this is also used for positive approximate and explicit plus numbers.

sffc

Great work!

feat: add percent formatter

5ace6c2

blaynem requested a review from younies as a code owner July 17, 2024 20:30

sffc reviewed Jul 17, 2024

View reviewed changes

blaynem added 2 commits July 18, 2024 11:05

percent datagen now uses icu_pattern

915aaa4

update debug data

71d0b84

blaynem requested review from robertbastian and Manishearth as code owners July 18, 2024 18:05

blaynem added 4 commits July 18, 2024 11:06

Merge branch 'main' of https://github.com/unicode-org/icu4x

99aecef

Merge branch 'main' into add-percent-formatter

8f98f63

update experimental bakeddata

5e73e28

add note for generated baked data for experimental dir

acf2c18

blaynem requested a review from a team as a code owner July 18, 2024 19:16

blaynem commented Jul 18, 2024

View reviewed changes

blaynem added 2 commits July 18, 2024 14:08

feat: percent fmt now handles negative patterns

544451c

use char instead of string const

7dec64d

blaynem marked this pull request as draft July 18, 2024 21:53

blaynem added 3 commits July 19, 2024 15:49

rework percent formatter to handle approximate, standard, and explici…

8a5ef1f

…t plus pattern

missed these 2

4bbbb1a

added generated data

bf21ea7

blaynem commented Jul 19, 2024

View reviewed changes

blaynem added 3 commits July 19, 2024 16:21

add missing imports

b4865e1

Merge branch 'main' of https://github.com/unicode-org/icu4x

3b1fd23

Merge branch 'main' into add-percent-formatter

4815066

blaynem commented Jul 19, 2024

View reviewed changes

address build failures

2140db4

blaynem marked this pull request as ready for review July 20, 2024 18:20

using wrong quotes for string and char

5f8645b

robertbastian reviewed Jul 24, 2024

View reviewed changes

blaynem added 2 commits July 24, 2024 18:25

address pr comments

f78e82f

Merge remote-tracking branch 'upstream/main' into add-percent-formatter

6caec17

sffc requested changes Jul 25, 2024

View reviewed changes

robertbastian previously approved these changes Jul 25, 2024

View reviewed changes

blaynem added 2 commits July 25, 2024 10:17

replace positive / negative pattern names with signed / unsigned

96c3a0a

percent now has correctly formatted decimals

7a7314e

blaynem dismissed robertbastian’s stale review via 7a7314e July 25, 2024 18:58

blaynem added 2 commits July 25, 2024 12:06

call try_new_unstable for passed provider

059033c

Merge branch 'main' into add-percent-formatter

b427846

blaynem mentioned this pull request Jul 26, 2024

feat: currency formatter now uses the fixedDecimalFormatter #5315

Merged

sffc approved these changes Jul 27, 2024

View reviewed changes

sffc merged commit 46aace9 into unicode-org:main Jul 27, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Percent Formatter to Experimental #5255

Adding Percent Formatter to Experimental #5255

blaynem commented Jul 17, 2024 •

edited

Loading

sffc Jul 17, 2024

blaynem Jul 18, 2024

blaynem Jul 19, 2024

robertbastian Jul 24, 2024

blaynem Jul 19, 2024

robertbastian Jul 24, 2024

blaynem Jul 24, 2024

robertbastian Jul 24, 2024

robertbastian Jul 24, 2024

blaynem Jul 25, 2024

robertbastian Jul 24, 2024

blaynem Jul 25, 2024

sffc left a comment

sffc Jul 25, 2024

blaynem Jul 25, 2024

robertbastian left a comment

robertbastian Jul 25, 2024

sffc left a comment

		/// https://www.unicode.org/reports/tr35/tr35-numbers.html#approximate-number-formatting
		/// https://www.unicode.org/reports/tr35/tr35-numbers.html#explicit-plus-signs

		let result = format_percent(&locale, Default::default(), positive_value);
		assert_eq!(result.as_str(), "12345.67%");

-        let result = format_percent(&locale, Default::default(), positive_value);
-        assert_eq!(result.as_str(), "12345.67%");
+        let default_fmt = PercentFormatter::try_new(&locale, Default::default()).unwrap();
+        let formatted_percent = default_fmt.format_percent(&positive_value);
+        assert_writeable_eq!(formatted_percent, "12345.67%");

Adding Percent Formatter to Experimental #5255

Adding Percent Formatter to Experimental #5255

Conversation

blaynem commented Jul 17, 2024 • edited Loading

What that PR doin?

Percent Subpatterns

Display Options

Approximate

Explicit Plus

Open Discussion

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

blaynem commented Jul 17, 2024 •

edited

Loading