Implement `SignedFixedDecimal` and `UnsignedFixedDecimal` #5667

younies · 2024-10-10T13:11:05Z

Related Issues: #5065

sffc · 2024-10-14T22:58:48Z

Good start; try to stick with the names we discussed in the thread. FixedDecimal and Signed<FixedDecimal>

…nd truncation methods public

…in parsing

… in FormattedDuration

…sing API entries

…tions

…orials

…in HeterogenousToFormatter

…tter in JS tiny loader

…dDecimal

…d distinctions

…remove redundant sections

sffc

Good work

sffc · 2024-11-15T07:19:30Z

components/datetime/src/format/datetime.rs

+            {
+                let mut num = SignedFixedDecimal::from(related_iso);
+                num.pad_start(l.to_len() as i16);
+                num
+            }


Issue: Why did you change padded_start to pad_start?

However, you'll need the { ... } block anyway if SignedFixedDecimal doesn't duplicate the functions of the inner type. Maybe we can just duplicate the -ed functions.

We discussed the -ed functions in June: #2902 (comment)

Because padded_start returns UnsignedFixedDecimal, however, pad_start changes the UnsignedFixedDecimal inside the UnsignedFixedDecimal.

sffc · 2024-11-15T07:25:46Z

tutorials/c-tiny/fixeddecimal/Cargo.lock

@@ -5,7 +5,7 @@ version = 3
 [[package]]
 name = "diplomat"
 version = "0.8.0"
-source = "git+https://github.com/rust-diplomat/diplomat?rev=8744ac97162341f347b63131969ad736e1047f6d#8744ac97162341f347b63131969ad736e1047f6d"
+source = "git+https://github.com/rust-diplomat/diplomat?rev=b49d63ec2a50a456b4b6e9d072d3650dca153ff8#b49d63ec2a50a456b4b6e9d072d3650dca153ff8"


Nit: Try not to include Cargo.lock changes in PRs with other functional changes. This Cargo.lock file might be stale, but it should be fixed in a standalone PR.

sffc · 2024-11-15T07:26:13Z

tutorials/c/Cargo.lock

@@ -47,7 +47,7 @@ dependencies = [
 [[package]]
 name = "diplomat"
 version = "0.8.0"
-source = "git+https://github.com/rust-diplomat/diplomat?rev=fe5201dd04ee6614942f817b291e90cb01440c19#fe5201dd04ee6614942f817b291e90cb01440c19"


Nit: Avoid lockfile diffs

sffc · 2024-11-15T07:28:24Z

utils/fixed_decimal/src/compact.rs

@@ -19,13 +19,13 @@ use crate::ParseError;
 /// nor a sign in the exponent, and behaves differently in pluralization.
 #[derive(Debug, Clone, PartialEq)]
 pub struct CompactDecimal {
-    significand: FixedDecimal,
+    significand: SignedFixedDecimal,


We should eventually make Compact composable, so that you can have an UnsignedCompactDecimal, for example. We can do that later in a different PR.

sffc · 2024-11-15T07:41:53Z

utils/fixed_decimal/src/decimal.rs

@@ -3167,10 +2527,6 @@ fn test_from_str_scientific() {
        pub output: &'static str,
    }
    let cases = [
-        TestCase {
-            input_str: "-5.4e10",


Suggestion: Add back some of these cases as failure cases to demonstrate that UnsignedFixedDecimal will fail to parse strings with a minus sign

sure, and for the ones starting with + too.

sffc · 2024-11-15T07:43:27Z

utils/fixed_decimal/src/decimal.rs

+        // Check if the output string starts with a plus sign and remove it,
+        // because we do not show sign for unsigned fixed decimal


Suggestion: maybe just reject strings with a + sign in UnsignedRoundingMode. It's okay to be more strict.

Okay, I agree with you.

As Rust behavior with unsigned types is as follows:

fn main() { let x: u32; x = +20; println!("x is: {}", x); }

This results in the following error:

error: leading `+` is not supported --> src/main.rs:3:9 | 3 | x = +20; | ^ unexpected `+` | help: try removing the `+` | 3 - x = +20; 3 + x = 20; |

…edDecimal

…al to the documentation

…used cases

sffc

LGTM; please sync to the latest main branch

younies added 5 commits October 8, 2024 16:49

First step into implementing the sign fixed decimal

b5517cb

implement SignedDecimal

c557cc0

Move important functions

89ee37f

Fix most unsinged int issues

8194482

Merge remote-tracking branch 'upstream/main' into add-sign-fixeddecimal

1ec4607

younies added 24 commits October 17, 2024 14:47

Merge remote-tracking branch 'upstream/main' into add-sign-fixeddecimal

9c52868

fix fmt

f3a4e1e

Add from_no_sign

0819754

Refactor module imports in fixed_decimal crate

b292bc7

Refactor module imports and add rounding module

797a27e

Merge remote-tracking branch 'upstream/main' into add-sign-fixeddecimal

3488f02

Merge remote-tracking branch 'upstream/main' into add-sign-fixeddecimal

efaeb0b

Merge remote-tracking branch 'upstream/main' into add-sign-fixeddecimal

d0378db

Refactor decimal module and rounding logic

69f9a96

Refactor decimal module and rounding logic

69a4602

Refactor decimal module and rounding logic: Make internal expansion a…

0170adf

…nd truncation methods public

Refactor decimal module and rounding logic: Improve handling of sign …

eeb6c0b

…in parsing

fix the write to

b17592e

Import the needed test cases and functions

f98341e

fix FrixedInteger

6f4bfa1

fix decimal.rs

6f6c68e

fix fixed integer

55a8074

copy rounding to signed_rounding

0f3b6e8

fix signed_rounding

401a9fd

fix rounding.rs

6cd257b

small fixes

87734ef

fix the formatter of fixed decimal

522969f

Fix compact decimal

2dab6ae

update permyriad.rs

6b2f7cf

younies added 15 commits November 12, 2024 18:45

fix fmt

459a608

Merge remote-tracking branch 'upstream/main' into add-sign-fixeddecimal

c68e117

Introduce a constructor for SignedFixedDecimal and refactor its usage…

989224e

… in FormattedDuration

cargo make diplomat-coverage

6661a58

Merge remote-tracking branch 'upstream/main' into add-sign-fixeddecimal

ac916bd

Update references from FixedDecimal to SignedFixedDecimal and add mis…

d98a628

…sing API entries

Replace FixedDecimal with SignedFixedDecimal in tests and format func…

630ee22

…tions

Replace FixedDecimal with SignedFixedDecimal in fixeddecimal.cpp

2156d7f

Replace FixedDecimal with SignedFixedDecimal in JS and TypeScript tut…

772c685

…orials

Update documentation to replace FixedDecimal with SignedFixedDecimal …

5217ac0

…in HeterogenousToFormatter

Replace FixedDecimal with SignedFixedDecimal in ICU test

226616e

Replace FixedDecimal with SignedFixedDecimal in JS tiny loader

9d4b32f

Replace SignedFixedDecimalFormatter references with FixedDecimalForma…

c11070c

…tter in JS tiny loader

Refactor benchmarks to use SignedFixedDecimal instead of UnsignedFixe…

24dbcb4

…dDecimal

Update documentation for rounding modes to clarify signed and unsigne…

b9a202d

…d distinctions

younies requested a review from sffc November 13, 2024 23:35

Refactor documentation for SignedFixedDecimal to enhance clarity and …

a696144

…remove redundant sections

sffc reviewed Nov 15, 2024

View reviewed changes

younies added 8 commits November 19, 2024 01:05

Merge remote-tracking branch 'upstream/main' into add-sign-fixeddecimal

def35eb

Replace FixedDecimal with SignedFixedDecimal in various components

accee4a

Refactor format function signature for improved readability

236de88

Update documentation to reflect change from FixedDecimal to SignedFix…

e38f7b6

…edDecimal

Add missing API entries for SignedFixedDecimal and UnsignedFixedDecim…

9b56fa5

…al to the documentation

Clarify error handling for sign characters in UnsignedFixedDecimal input

a50c928

Add test cases for syntax errors in decimal input strings

87dfd16

Remove leading plus sign from test case input strings and clean up un…

2dd420f

…used cases

younies requested a review from sffc November 19, 2024 01:16

younies added 2 commits November 20, 2024 18:37

Merge remote-tracking branch 'upstream/main' into add-sign-fixeddecimal

98e0518

Update dependencies and versions in Cargo.lock

ff7706c

sffc approved these changes Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `SignedFixedDecimal` and `UnsignedFixedDecimal` #5667

Implement `SignedFixedDecimal` and `UnsignedFixedDecimal` #5667

younies commented Oct 10, 2024 •

edited

Loading

sffc commented Oct 14, 2024

sffc left a comment

sffc Nov 15, 2024

younies Nov 18, 2024

sffc Nov 15, 2024

sffc Nov 15, 2024

younies Nov 20, 2024

sffc Nov 15, 2024

sffc Nov 15, 2024

younies Nov 19, 2024

sffc Nov 15, 2024

younies Nov 19, 2024

sffc left a comment

		// Check if the output string starts with a plus sign and remove it,
		// because we do not show sign for unsigned fixed decimal

Implement SignedFixedDecimal and UnsignedFixedDecimal #5667

Are you sure you want to change the base?

Implement SignedFixedDecimal and UnsignedFixedDecimal #5667

Conversation

younies commented Oct 10, 2024 • edited Loading

sffc commented Oct 14, 2024

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

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

Implement `SignedFixedDecimal` and `UnsignedFixedDecimal` #5667

Implement `SignedFixedDecimal` and `UnsignedFixedDecimal` #5667

younies commented Oct 10, 2024 •

edited

Loading