Implement LinearConverter
#4404
Conversation
| /// The conversion is not possible due to an internal error. | ||
| InternalError, | ||
| /// The needed data is not found. | ||
| DataNotFoundError, |
There was a problem hiding this comment.
Because there's information in the DataError that you should propagate. Just to be clear, I'm saying this variant should be
| DataNotFoundError, | |
| Data(DataError), |
However, as far as I can tell this is actually unused if you address all my comments.
| match si_prefix.base { | ||
| Base::Decimal => { | ||
| *ratio *= Ratio::<BigInt>::from_integer(10.into()).pow(si_prefix.power as i32); | ||
| } | ||
| Base::Binary => { | ||
| *ratio *= Ratio::<BigInt>::from_integer(2.into()).pow(si_prefix.power as i32); | ||
| } | ||
| } |
There was a problem hiding this comment.
No, core::ops::Mul does not have to be with the same type. Because this involves a mutable type you'd have to use core::ops::MulAssign though:
impl core::ops::MulAssign<SiPrefix> for IcuRatio {
fn mul_assign(&mut self, x: Ratio) -> {
...
}
}This is the idiomatic way to do this. You can then write:
conversion_info_factor *= unit.item.si_prefix;This requires having an IcuRatio type as @sffc suggested.
| let sign = match Sign::from_unaligned(*sign_ule) { | ||
| Sign::Positive => num_bigint::Sign::Plus, | ||
| Sign::Negative => num_bigint::Sign::Minus, | ||
| }; |
There was a problem hiding this comment.
The rusty way to do this:
impl Into<num_bigint::Sign> for Sign {
...
}
| /// NOTE: | ||
| /// The converter is not able to convert between two units that are not single. such as "foot-and-inch" to "meter". | ||
| #[derive(Debug)] | ||
| pub struct LinearConverter { |
There was a problem hiding this comment.
Yes, sounds like a bikeshed for later
| match si_prefix.base { | ||
| Base::Decimal => { | ||
| *ratio *= Ratio::<BigInt>::from_integer(10.into()).pow(si_prefix.power as i32); | ||
| } | ||
| Base::Binary => { | ||
| *ratio *= Ratio::<BigInt>::from_integer(2.into()).pow(si_prefix.power as i32); | ||
| } | ||
| } |
There was a problem hiding this comment.
But it's raising to a power, not multiplying
| match si_prefix.base { | ||
| Base::Decimal => { | ||
| *ratio *= Ratio::<BigInt>::from_integer(10.into()).pow(si_prefix.power as i32); | ||
| } | ||
| Base::Binary => { | ||
| *ratio *= Ratio::<BigInt>::from_integer(2.into()).pow(si_prefix.power as i32); | ||
| } | ||
| } |
There was a problem hiding this comment.
But it's raising to a power, not multiplying
It is multiplying something with the SI prefix. That the SI prefix is a power of 2/10 is internal to the SI prefix.
robertbastian
changed the title
LinearConverter
No description provided.