Skip to content
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

Adds Decimal methods to access the coefficient and exponent #593

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

zslayton
Copy link
Contributor

@zslayton zslayton commented Jul 5, 2023

This PR:

  • Makes Coefficient a publicly visible type.
  • Adds a Decimal::coefficient method to access the coefficient.
  • Adds a Decimal::exponent method to access the exponent.
  • Makes the decimal module a directory instead of a flat file
  • Moves the Magnitude newtype to its own module
  • Conversions to/from Int, UInt, and Decimal now return IonError instead of () in the event of a failure.

Fixes #405.

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

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Patch coverage: 72.36% and project coverage change: -0.02 ⚠️

Comparison is base (a7e0f03) 82.88% compared to head (44ca6aa) 82.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
- Coverage   82.88%   82.87%   -0.02%     
==========================================
  Files         110      111       +1     
  Lines       19250    19365     +115     
  Branches    19250    19365     +115     
==========================================
+ Hits        15956    16049      +93     
- Misses       1751     1773      +22     
  Partials     1543     1543              
Impacted Files Coverage Δ
src/lib.rs 33.33% <ø> (ø)
src/types/decimal/mod.rs 91.94% <0.00%> (ø)
src/types/integer.rs 79.44% <0.00%> (-2.31%) ⬇️
src/types/mod.rs 96.49% <ø> (ø)
src/types/decimal/coefficient.rs 89.57% <86.27%> (ø)
src/types/decimal/magnitude.rs 88.00% <88.00%> (ø)

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

Copy link
Contributor Author

@zslayton zslayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ PR tour

///
/// Signed integers can infallibly implement `Into<Magnitude>` by using their
/// absolute value.
pub struct Magnitude(UInt);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ This type was moved into its own module. You'll see its definition appear further on.

self.is_zero_with_sign(Sign::Positive)
}

pub(crate) fn is_zero_with_sign(&self, test_sign: Sign) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ This helper method is used in the implementation of both is_negative_zero() and is_positive_zero().

Users may wish to use is_negative_zero() before converting the Coefficient to an Int, as Int cannot represent negative zero natively. is_positive_zero() is provided mostly for symmetry.

///
/// Signed integers can infallibly implement `Into<Magnitude>` by using their
/// absolute value.
pub struct Magnitude(UInt);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ As mentioned above, this type was moved to its own module. It contains no new code apart from the From<BigUint> for Magnitude impl on line 32.

type Error = ();
type Error = IonError;
fn try_from(value: $t) -> Result<Self, Self::Error> {
if value < 0 {
return Err(());
return IonResult::illegal_operation("cannot convert a negative number to a UInt");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Previously, many conversions that would fail used () as their error type because the single mode of failure was obvious. (For example: trying to convert a negative Int to a UInt.) However, making these into IonError means they can be bubbled up with ? alongside all other IonResult-producing code.

illegal_operation isn't ideal; I'm open to creating an InvalidConversion error variant, but I don't expect users to will inspect conversion errors very deeply.

@zslayton zslayton requested review from popematt and desaikd July 5, 2023 17:36
@zslayton zslayton marked this pull request as ready for review July 5, 2023 17:37
@zslayton zslayton merged commit 9c19ab6 into main Jul 5, 2023
@zslayton zslayton deleted the decimal-coefficient branch July 5, 2023 18:36
);

// BigInt
let enormous_int = BigInt::from_str("1234567890123456789012345678901234567890").unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for fun, this is a trick I sometimes use when making strings of numbers for length:

Suggested change
let enormous_int = BigInt::from_str("1234567890123456789012345678901234567890").unwrap();
let enormous_int = BigInt::from_str("1234567891023456789202345678930234567894").unwrap();

This allows you to more easily figure out how long the string is, find a 9x0 and the digit x is the 10s place not a zero. The following 1 is not valuable (inferable from 234..), so I write 20, 30, what have you instead.

Comment on lines +19 to 20

/// An arbitrary-precision Decimal type with a distinct representation of negative zero (`-0`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth including some comment about what negative zero is for, what it means?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read value of a Decimal
4 participants