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

SignedDecimal implementation #1807

Merged
merged 26 commits into from
Sep 25, 2023
Merged

SignedDecimal implementation #1807

merged 26 commits into from
Sep 25, 2023

Conversation

chipshort
Copy link
Collaborator

@chipshort chipshort commented Aug 4, 2023

closes #1711

Needs #1870 to work

Current status: Both SignedDecimal and SignedDecimal256 are finished

  • I added negative cases to almost all tests
  • I had to adjust these methods (compared to Decimal): from_str, saturating_{add, sub, mul, pow}, to_int_{floor, ceil}
  • I added some new methods: trunc and to_int_trunc
  • I removed sqrt, as we don't have it for the signed ints atm and it would have to return a Result in case of negative numbers

@webmaster128
Copy link
Member

Adding this to 1.4 for visibility, but would not consider it a hard release blocker. If we can get it in, great.

@webmaster128 webmaster128 modified the milestones: 1.4.0, 2.0.0 Aug 22, 2023
@chipshort chipshort force-pushed the 1711-signed-decimals branch 3 times, most recently from 7fdf484 to aea5e42 Compare September 1, 2023 12:37
@webmaster128
Copy link
Member

Looks very good

I removed sqrt, as we don't have it for the signed ints atm and it would have to return a Result in case of negative numbers

I think it's fine to not have that. Square root of negative numbers only work in complex math. I think users can be asked to convert to unsigned first and ensure errors are handles in that step.

@chipshort chipshort changed the base branch from main to release/1.4 September 19, 2023 13:27
@chipshort chipshort changed the base branch from release/1.4 to main September 19, 2023 13:27
@chipshort chipshort marked this pull request as ready for review September 19, 2023 13:27
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Let's add a CHANGELOG entry here and then merge 🚀

@chipshort chipshort merged commit 1e1eb5e into main Sep 25, 2023
27 checks passed
@chipshort chipshort deleted the 1711-signed-decimals branch September 25, 2023 14:08
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.

Signed Decimals
3 participants