-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add API for padding to a minimum number of decimal digits #1482
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like the FixedDecimal class to be robust against integer overflow because we have actually had problems with this in ICU. Whenever you add, subtract, or cast these things, you should handle the overflow. See my suggestions. Ideally we also have test cases for these.
Co-authored-by: Shane F. Carr <shane@unicode.org>
Co-authored-by: Shane F. Carr <shane@unicode.org>
} | ||
|
||
// how many digits need to be deleted from the start. | ||
let cut = self.magnitude - self.upper_magnitude; | ||
self.upper_magnitude = magnitude; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: add
#[cfg(debug_assertions)]
self.check_invariants();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Please add here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not here yet. Please add the check_invariants
in all three functions (pad left, pad right, truncate left).
Also please fix the clippy warning. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after adding the debug assertions.
Also optionally add unit tests for magnitudes near i16::MAX to test the new branches, or open an issue to add these in a follow-up.
Filed #1496 for now |
Fixes #1476
I'm not sure what to name this. Honestly I think that we should name this API
minimum_decimal_digits
instead (and maybe get rid of the padding API?). I don't see truncation as particularly useful. Overall from talking to the WearOS users they do prefer to think in terms of "minimum" and "maximum" digits.