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

FixedDecimal *ed methods should be infallible #4857

Open
sffc opened this issue May 2, 2024 · 5 comments
Open

FixedDecimal *ed methods should be infallible #4857

sffc opened this issue May 2, 2024 · 5 comments
Labels
2.0-breaking Changes that are breaking API changes C-numbers Component: Numbers, units, currencies S-small Size: One afternoon (small bug fix or enhancement)
Milestone

Comments

@sffc
Copy link
Member

sffc commented May 2, 2024

This function is misleading:

pub fn concatenated_end(self, other: FixedDecimal) -> Result<Self, FixedDecimal>

In the error case, it returns other, but it still throws away self. It should probably return a tuple containing both FixedDecimals. It makes it tempting to write code such as mantissa.concatenated_end(fraction).unwrap_or_else(|fd| fd), which is wrong because it promotes fraction when it should probably keep mantissa and throw away fraction.

I think we should delete this function since it is only sugar over concatenate_end, which is harder to use incorrectly.

@sffc sffc added C-numbers Component: Numbers, units, currencies S-small Size: One afternoon (small bug fix or enhancement) 2.0-breaking Changes that are breaking API changes labels May 2, 2024
@sffc sffc added this to the ICU4X 2.0 milestone May 2, 2024
@robertbastian
Copy link
Member

For the datetime use case, I think it would be useful to have a FixedDecimal constructor that accepts usize, usize as the integer and fractional parts. Then we avoid this infallibility altogether.

@sffc
Copy link
Member Author

sffc commented May 2, 2024

For the datetime use case, I think it would be useful to have a FixedDecimal constructor that accepts usize, usize as the integer and fractional parts. Then we avoid this infallibility altogether.

I don't think it solves the overlap issue.

@robertbastian
Copy link
Member

I think it does. It would use IntIterator::from(integer).chain(IntIterator::from(fractional)) and then shift the resulting FixedDecimal by fractional.log10().

@sffc
Copy link
Member Author

sffc commented May 3, 2024

Hmm, that's quite clever. How does it handle these cases though?

  • Seconds = 123, Nanoseconds = 20 (needs to know that there are leading zeros: easy enough)
  • Seconds = 123, Nanoseconds = 4444444444 (same problem as we currently have: overlapping fields)

@sffc
Copy link
Member Author

sffc commented May 3, 2024

I wouldn't be opposed to implementing a plus operator that just performs a sum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0-breaking Changes that are breaking API changes C-numbers Component: Numbers, units, currencies S-small Size: One afternoon (small bug fix or enhancement)
Projects
Status: No status
Development

No branches or pull requests

2 participants