add pad and padded to FixedDecimal #1195

younies · 2021-10-20T21:00:28Z

sffc

Yep, this is about right, but let's sync on the details

utils/fixed_decimal/src/decimal.rs

tdenniston · 2021-10-21T18:48:47Z

utils/fixed_decimal/src/decimal.rs

+    /// dec.pad(2);
+    /// assert_eq!("042", dec.to_string());
+    ///
+    /// dec.pad(-2);


Would this be additive with multiply_pow10()? E.g. suppose I had:

let mut dec = FixedDecimal::from(1234); dec.multiply_pow10(-1); // "123.4" dec.pad(-2); // "123.400" ?

I wonder if we should be normalizing before we pad, i.e. convert everything into (n digits) + n magnitude.

We should definitely add doctests for the behavior here

Or, well, if minus padding, we first put it in a form where it is not already padded on the right, and if plus-padding, put it in a form where it's not already padded on the left

Actually wait I think the data model doesn't need this 😄

sffc · 2021-10-22T00:26:29Z

utils/fixed_decimal/src/decimal.rs

+        let cut = self.magnitude - self.upper_magnitude;
+
+        self.magnitude = self.upper_magnitude;
+        self.digits.drain(0..(cut as usize));


Nit: Please add a test case that triggers the case where cut is larger than self.digits.len()

sffc · 2021-10-22T00:28:47Z

utils/fixed_decimal/src/decimal.rs

-            Ordering::Greater => self.upper_magnitude = magnitude,
-            _ => (),
+    pub fn pad_or_trunc_left(&mut self, shift: i16) {
+        self.upper_magnitude = match self.upper_magnitude + shift >= 0 {


Nit: Please use checked_add instead of +. You may need to make the function return Result; see multiply_pow10. Add a test case for this behavior at the bottom of the file.

sffc · 2021-10-22T00:33:34Z

utils/fixed_decimal/src/decimal.rs

-            Ordering::Less => self.lower_magnitude = magnitude,
-            Ordering::Greater => self.upper_magnitude = magnitude,
-            _ => (),
+    pub fn pad_or_trunc_left(&mut self, shift: i16) {


Suggested change

pub fn pad_or_trunc_left(&mut self, shift: i16) {

pub fn pad_or_truncate_left(&mut self, shift: i16) {

sffc · 2021-10-22T00:33:44Z

utils/fixed_decimal/src/decimal.rs

    /// ```
-    pub fn padded(mut self, magnitude: i16) -> Self {
-        self.pad(magnitude);
+    pub fn padded_or_trunced_left(mut self, magnitude: i16) -> Self {


Suggested change

pub fn padded_or_trunced_left(mut self, magnitude: i16) -> Self {

pub fn padded_or_truncated_left(mut self, magnitude: i16) -> Self {

utils/fixed_decimal/src/decimal.rs

sffc · 2021-10-26T03:21:08Z

utils/fixed_decimal/src/decimal.rs

+    /// assert_eq!("000", dec.to_string());
+    ///
+    /// dec.pad_or_truncate_left(-1000);
+    /// assert_eq!("", dec.to_string());


Issue: don't put the FixedDecimal in a state where it returns empty strings.

so, what should we do if the user is deleting all the numbers? shall we consider returning an error?

In my opinion, we should leave a 0 in the ones place and never remove it.

0042.10 --> 042.10 --> 42.10 --> 2.10 --> 0.10

We could consider allowing the removal of the zero digit, but I'd like that to be a separate PR, because right now FixedDecimal is defined in such a way that we always guarantee at least one digit.

sffc · 2021-10-26T03:21:55Z

utils/fixed_decimal/src/decimal.rs

+    /// dec.pad_or_truncate_left(-1);
+    /// assert_eq!("2", dec.to_string());
+    ///
+    /// let mut dec = FixedDecimal::from(1000);


Nit: The exhaustive behavior tests should go in a unit test at the bottom of the file. Only use docs tests for the main illustrative tests.

…string.

Manishearth

So this looks good, but it occurs to me that the user may need non-padding truncation as well (WearOS does)

I think we need functions that provide both:

the ability to say "make this have exactly X digits before/after the decimal point"
the ability to say "make this have at most X digits after the decimal point"
the ability to say "make this have at least X digits before the decimal point"

I don't think truncating on the LHS is that useful, and I think padding on the RHS is somewhat useful but you need a way to opt out.

I wonder if the API should be:

pad_left()
truncate_right()
pad_or_truncate_right()

We could also get by with pad(), truncate(), and pad_or_truncate() all accepting negative integers though I don't see much of a use case for each one so I'd prefer having them, by default, working on the direction this is more useful for. @sffc thoughts?

sffc · 2021-10-30T05:09:29Z

@Manishearth #1176 has my thoughts on the current proposed algorithm. The API that @younies proposed already allows for either either left* padding (positive argument) or left truncation (negative argument). Right* padding and right truncation should be handled by #1177.

A use case for left truncation is removing the century from year values, like in short date formatting ("today's date is 11/29/21").

* I say "left" and "right" even though certain scripts like Adlam write their digits in the opposite direction. I'm happy to bikeshed the name of these functions.

add pad and padded to FixedDecimal

da2be3a

younies requested a review from sffc as a code owner October 20, 2021 21:00

sffc requested changes Oct 20, 2021

View reviewed changes

utils/fixed_decimal/src/decimal.rs Outdated Show resolved Hide resolved

tdenniston reviewed Oct 21, 2021

View reviewed changes

Make it pad or trunc@

ec88b87

younies requested a review from sffc October 21, 2021 23:22

sffc requested changes Oct 22, 2021

View reviewed changes

Fix edge cases and add more tests

c01ee96

younies requested a review from sffc October 26, 2021 01:56

sffc requested changes Oct 26, 2021

View reviewed changes

add an extensive tests and fix the case which the number is an empty …

f44d66a

…string.

younies requested a review from sffc October 26, 2021 23:34

Manishearth approved these changes Oct 27, 2021

View reviewed changes

Manishearth mentioned this pull request Oct 29, 2021

Double-to-decimal in ICU4X #166

Closed

sffc approved these changes Oct 30, 2021

View reviewed changes

younies merged commit 11288c3 into unicode-org:main Nov 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add pad and padded to FixedDecimal #1195

add pad and padded to FixedDecimal #1195

younies commented Oct 20, 2021 •

edited by sffc

sffc left a comment

tdenniston Oct 21, 2021

Manishearth Oct 21, 2021

Manishearth Oct 21, 2021

Manishearth Oct 21, 2021

sffc Oct 22, 2021

younies Oct 26, 2021

sffc Oct 22, 2021

younies Oct 26, 2021

sffc Oct 22, 2021

younies Oct 26, 2021

sffc Oct 22, 2021

younies Oct 26, 2021

sffc Oct 26, 2021

younies Oct 26, 2021

sffc Oct 26, 2021

sffc Oct 26, 2021

younies Oct 26, 2021

sffc Oct 26, 2021

younies Oct 26, 2021

Manishearth left a comment •

edited

sffc commented Oct 30, 2021

	pub fn pad_or_trunc_left(&mut self, shift: i16) {
	pub fn pad_or_truncate_left(&mut self, shift: i16) {

	pub fn padded_or_trunced_left(mut self, magnitude: i16) -> Self {
	pub fn padded_or_truncated_left(mut self, magnitude: i16) -> Self {

add pad and padded to FixedDecimal #1195

add pad and padded to FixedDecimal #1195

Conversation

younies commented Oct 20, 2021 • edited by sffc

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

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

Manishearth left a comment • edited

Choose a reason for hiding this comment

sffc commented Oct 30, 2021

younies commented Oct 20, 2021 •

edited by sffc

Manishearth left a comment •

edited