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

Add PG::Numeric#to_big_d #296

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jgaskins
Copy link
Contributor

@jgaskins jgaskins commented Dec 3, 2024

Exploring using decimal columns for storing currency values and representing them with BigDecimal values in Crystal code, but then noticed BigDecimal support hasn't made it into this shard yet. When PG::Numeric#to_big_r was added, I don't think BigDecimal existed in the Crystal stdlib.

I copied most of the code from to_big_r, but rather than building up a power of 10 for the second argument based on the placement of the decimal point, we can just use the placement of the decimal point itself.

A couple of the tests are failing and I'm not sure how to fix them yet. They look like trivial ones, but I spent too long trying to figure out how BigDecimal` works and I'm out of gas for the evening.

@jgaskins
Copy link
Contributor Author

jgaskins commented Dec 3, 2024

A couple of the tests are failing and I'm not sure how to fix them yet.

j/k the expected outputs were wrong

Copy link
Collaborator

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

Looks great.

Don't we also need a way to encode BigDecimal as a parameter?

@will
Copy link
Owner

will commented Dec 3, 2024

Thanks @jgaskins ! Could you also please add a changelog item?

@jgaskins
Copy link
Contributor Author

jgaskins commented Dec 3, 2024

Don't we also need a way to encode BigDecimal as a parameter?

For #295, absolutely. Until then, is the catch-all sufficient?

Could you also please add a changelog item?

Sure thing!

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.

3 participants