Make Display for Decimal more human-friendly #477

jobarr-amzn · 2023-03-10T00:36:14Z

Description of changes:

Before this all Decimals would be displayed as e.g. 37d-2 instead of 0.37. After this decimals are displayed as ABC.DEF, ABCd3 or 0.000ABC unless the number of padding zeroes grows large, in which case it will fall back to A.BCd-E for some exponent.

This also prefers to quantify the scale of larger numbers, e.g. ABCDEFGd0 will be written as A.BCDEFGd6 instead of ABCDEFG.

"large" is henceforth "six" until someone changes it ;)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

popematt · 2023-03-10T06:01:24Z

src/types/decimal.rs

+        // The index of the decimal
+        let i_d = len as i64 + self.exponent;


What does "index of the decimal" mean?

I added a comment:

// The index of the decimal, relative to the magnitude representation // 0123 // Given ABCDd-2, the decimal gets inserted at position 2. let i_d = len as i64 + self.exponent;

popematt · 2023-03-10T06:02:56Z

src/types/decimal.rs

+            write!(f, "{}.{}d{}", &digits[0..1], &digits[1..len], (i_d - 1))
+        } else if self.exponent > 0 {
+            // e.g. ABC0000. or ABC.
+            let zeroes = "0".repeat(self.exponent as usize);


This line is adding more digits of precision to the output.

Good callout. I've fixed it and the tests pass now.

codecov · 2023-03-10T06:17:37Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (e3f7bd1) 90.13% compared to head (9925c62) 90.16%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #477      +/-   ##
==========================================
+ Coverage   90.13%   90.16%   +0.03%     
==========================================
  Files          76       76              
  Lines       13548    13568      +20     
==========================================
+ Hits        12211    12234      +23     
+ Misses       1337     1334       -3

Impacted Files	Coverage Δ
src/types/decimal.rs	`96.18% <100.00%> (+0.28%)`	⬆️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/types/decimal.rs

popematt · 2023-03-10T06:22:09Z

src/types/decimal.rs

+                write!(f, "{}.{}", &digits[0..ui_d], &digits[ui_d..len])
+            } else if i_d > -6 {
+                // e.g. 0.ABC or 0.000ABC
+                let zeroes = "0".repeat(i_d.abs() as usize);


Can you please add Clippy's suggestions here?

I ran into rust-lang/rust-clippy#7416 and had a very frustrating time before I managed to get clippy running.

Fixed the cast_abs_to_unsigned issue.

Later obviated by Zack's format string suggestion.

zslayton · 2023-03-10T13:20:02Z

src/types/decimal.rs

+        // The index of the decimal, relative to the magnitude representation
+        //       0123
+        // Given ABCDd-2, the decimal gets inserted at position 2.
+        let i_d = len as i64 + self.exponent;


How about d_index?

Originally I used decimal_index, but then rustfmt forces some of the write! cases into multiple lines. I shortened some variable names to keep everything on one line because I found the multiline version so much less appealing.

d_index is similarly too long. I wish I could get rustfmt to tell me which constraint it's running into, because it seems to not fire when line length is 83 but does fire on line length 84? So maybe it's not line length, maybe it's length of parameter list? I don't know.

zslayton · 2023-03-10T13:24:44Z

src/types/decimal.rs

+            write!(f, "-").unwrap();
+        };
+
+        if self.exponent == 0 && len > 6 {


Could you make 6 a named constant and/or add a comment explaining how you chose it?

Will do both. Happy to change to some other value, as well.

src/types/decimal.rs

zslayton · 2023-03-10T13:27:29Z

src/types/decimal.rs

+            // exponent < 0, there is a fractional component
+            if i_d > 0 {
+                // e.g. A.BC or AB.C
+                let ui_d = i_d as usize;


If it's helpful, you can shadow the existing name in this scope so you don't have to think of a new one. (IMO, ui_d is a bit cryptic, though I gather it stands for unsigned_index_of_d?). Shadowing has the potential to be a bit confusing for readers outside of an IDE, but in this case it only lasts one line.

zslayton · 2023-03-10T13:32:42Z

src/types/decimal.rs

+                write!(f, "{}.{}", &digits[0..ui_d], &digits[ui_d..len])
+            } else if i_d > -6 {
+                // e.g. 0.ABC or 0.000ABC
+                let zeroes = "0".repeat(i_d.abs() as usize);


This will allocate a new string, but you can achieve the same thing using the format string you pass to write!. See the docs for Fill/Alignment and Sign/#/0 for details.

Alternatively, create one very long static ZEROS: &str = "0000000..."; and just take a slice of the desired length 😛

Using the format string passed to write! was a great suggestion, took a little fiddling but I like it :D

Co-authored-by: Matthew Pope <81593196+popematt@users.noreply.github.com>

Co-authored-by: Zack Slayton <zack.slayton@gmail.com>

zslayton

🚀 A nice improvement!

zslayton · 2023-03-10T21:41:01Z

src/types/decimal.rs

    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        // Inspired by the formatting conventions of Java's BigDecimal.toString()
+        const WIDE_NUMBER: i64 = 6; // if you think about it, six is a lot 🙃


This could be a usize, which would allow you to skip:

casting len as i64 down on line 308

casting WIDE_NUMBER as usize on line 314

Yeah WIDE_NUMBER should be a usize.

If I don't cast len as i64 on line 308 thendot_index gets inferred as usize, but that's not right because dot_index should be able to be negative.

As far as I understand it, I either have to cast WIDE_NUMBER as usize on line 314 to agree with len or cast WIDE_NUMBER as i64 on line 324 for easiest comparison with the negative-valued dot_index there. The latter is the right call because WIDE_NUMBER should be a usize.

Unless I'm missing something, which is probable :) I'd also happily take feedback or second opinions on the WIDE_NUMBER concept- whether we ought to have it at all, whether it ought to be applied "evenly" on both whole and fractional numbers, and how large it should be.

jobarr-amzn force-pushed the display-decimal branch from d8b9b26 to 97f63ac Compare March 10, 2023 00:58

popematt reviewed Mar 10, 2023

View reviewed changes

jobarr-amzn force-pushed the display-decimal branch from 97f63ac to 1661c9d Compare March 10, 2023 06:08

Make Display for Decimal more human-friendly

e4c3d5d

jobarr-amzn force-pushed the display-decimal branch from 1661c9d to e4c3d5d Compare March 10, 2023 06:15

popematt reviewed Mar 10, 2023

View reviewed changes

src/types/decimal.rs Outdated Show resolved Hide resolved

popematt reviewed Mar 10, 2023

View reviewed changes

zslayton reviewed Mar 10, 2023

View reviewed changes

jobarr-amzn and others added 3 commits March 10, 2023 07:19

Update src/types/decimal.rs

de86c80

Co-authored-by: Matthew Pope <81593196+popematt@users.noreply.github.com>

Update src/types/decimal.rs

bb418a5

Co-authored-by: Zack Slayton <zack.slayton@gmail.com>

PR feedback and skip rustfmt

bd82ad8

zslayton approved these changes Mar 10, 2023

View reviewed changes

WIDE_NUMBER as usize

9925c62

jobarr-amzn merged commit f49659e into main Mar 11, 2023

jobarr-amzn deleted the display-decimal branch March 11, 2023 00:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Display for Decimal more human-friendly #477

Make Display for Decimal more human-friendly #477

jobarr-amzn commented Mar 10, 2023 •

edited

popematt Mar 10, 2023

jobarr-amzn Mar 10, 2023

popematt Mar 10, 2023

jobarr-amzn Mar 10, 2023

codecov bot commented Mar 10, 2023 •

edited

popematt Mar 10, 2023

jobarr-amzn Mar 10, 2023

jobarr-amzn Mar 10, 2023

zslayton Mar 10, 2023

jobarr-amzn Mar 10, 2023 •

edited

zslayton Mar 10, 2023

jobarr-amzn Mar 10, 2023

zslayton Mar 10, 2023

zslayton Mar 10, 2023

jobarr-amzn Mar 10, 2023

zslayton left a comment

zslayton Mar 10, 2023

jobarr-amzn Mar 10, 2023

		// The index of the decimal
		let i_d = len as i64 + self.exponent;

Make Display for Decimal more human-friendly #477

Make Display for Decimal more human-friendly #477

Conversation

jobarr-amzn commented Mar 10, 2023 • edited

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

codecov bot commented Mar 10, 2023 • edited

Codecov Report

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

jobarr-amzn Mar 10, 2023 • edited

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

zslayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jobarr-amzn commented Mar 10, 2023 •

edited

codecov bot commented Mar 10, 2023 •

edited

jobarr-amzn Mar 10, 2023 •

edited