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 support for MySql::Type::Decimal #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

steveaffleck
Copy link

Added support for MySql::Type::Decimal as BigDecimal in crystal.

@bcardiff
Copy link
Member

bcardiff commented Apr 6, 2023

Thanks for contributing!

I think that the downside of using BigDecimal is that we will force a dependency on libgmp. Not 100% sure. It might also be a bit too much to use BigDecimal for MySQL decimals, but it's like we have a smaller alternative.

Possible outcomes:

  1. Check/Accept that we will have a libgmp dependency for apps using crystal-mysql. The decl_type will cause an entry in types_by_code so BigDecimal will always be required even if not used I think.
  2. Put the Decimal support backed with BigDecimal behind a feature flag or an optional require "mysql/big"
  3. Create a MySql::Decimal Number type that would be lighter than BigDecimal. This type could offer from/to methods for decimal and other types (at the expense of loosing precision). If the to_decimal are not used, then there is no dependency to libgmp.

Let's wait for other's feedback

@steveaffleck
Copy link
Author

For my use case, option 3 is probably best. I can imagine there are cases where precision could be more important. It isn't something I run into often.

@straight-shoota
Copy link
Member

Yeah, crystal-mysql should not depend on big (1). Support for DECIMAL can be optional (2) or implemented w/o BigDecimal (3).
I'm fine with either.
(3) Requires some implementation effort, while (2) requires an extra include when you want to use decimals. But both are manageable.

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