-
Notifications
You must be signed in to change notification settings - Fork 33
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
Removes vestigial dependency on BigDecimal #594
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #594 +/- ##
==========================================
- Coverage 82.87% 82.84% -0.04%
==========================================
Files 111 111
Lines 19365 19302 -63
Branches 19365 19302 -63
==========================================
- Hits 16049 15990 -59
+ Misses 1773 1770 -3
+ Partials 1543 1542 -1
☔ View full report in Codecov by Sentry. |
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.
🗺️ PR tour
src/element/reader.rs
Outdated
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.
🗺️ Most of the changes in this file are to use implicit conversions to avoid going from Rust type -> Value
-> Element
. They now go directly from Rust type -> Element
.
src/lib.rs
Outdated
pub mod external { | ||
pub use bigdecimal; | ||
} | ||
pub mod external {} |
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 think we still need this for dependencies behind opt-in feature gates.
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.
(Question) Can we remove this module as it doesn't have any dependencies at the moment? Later when we need re-exports for the opt-in features, we can create the module for it.
Having this in crate docs with no dependencies in it might be confusing, so we should either add a comment or an issue link stating why we have kept it or indicating why this is empty at the moment.
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.
Great point, I'll take it out.
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.
Given that crate A can't implement a trait from crate B for a type from crate C, we might eventually realize that we want to provide From<BigDecimal>
for Decimal
or Element
behind a feature flag.
...but I don't think that's any reason not to make the change.
Early versions of
ion-rust
used the third-partyBigDecimal
type to represent Iondecimal
values. The library has had its ownDecimal
type for a while now that provides a correct representation of negative zero and properly implements Ion equality testing.This PR removes the library's remaining uses of
BigDecimal
and drops the dependency.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.