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 Decimal #117

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

Conversation

leandrocp
Copy link

@leandrocp leandrocp commented Oct 4, 2021

First of all, thanks for creating this lib :)

I've been working with Decimal mostly because float uses the shortest representation according to the algorithm described in "Printing Floating-Point Numbers Quickly and Accurately" (quoting the doc https://hexdocs.pm/elixir/Float.html#to_string/1), ie:

iex> "#{Float.round(1.1, 2)}"
"1.1"
iex> "#{Decimal.round("1.1", 2)}"
"1.10"

Besides this presentation issue, converting to_string makes that cell to be a binary which isn't ideal for working with formulas on the sheet.

AFAIK this has been proposed before (#43) but it was rejected for 2 reasons: 1) no actual use-case for it, and 2) for adding the Decimal lib (looks like that's the main reason). Turns out there's no need to actually add a dependency, we can check if it's loaded at run-time, just like many libs do, eg in ecto_sql or phoenix_live_view. I believe that's reasonable so I'd like to propose adding Decimal handling yet one more time, considering there's a real use-case to justify adding it and there's no need to add a dependency.

/edit On repo https://github.com/leandrocp/elixlsx_decimal/commits/main you can see an example with and without Decimal

@unti1x
Copy link
Collaborator

unti1x commented Oct 6, 2021

Greetings

First of all, I don't think there is a real reason to support Decimal in the library. People who don't use it will receive constant load checks and, probably, conflicts. Dependency on Decimal is totally unnecessary but without one it turns out kinda obscure which exact Decimal module is required

@leandrocp
Copy link
Author

Hi @unti1x, thanks for the feedback.

I don't think there is a real reason to support Decimal in the library

Besides the reasons I've described, remember that Ecto supports Decimal out of the box so it's not rare to have queries returning decimal, especially if you have complex queries for reporting.

will receive constant load checks

Code.ensure_loaded? is a no-op when Decimal is already loaded, or just a fast op if not. Even so that popular libs like Ecto rely on it.

conflicts

Given that Decimal is a popular library, it's unlike that someone is gonna export a module called Decimal. But that kind of conflict can be mitigated by changing that guard to if Code.ensure_loaded?(Decimal) and function_exported?(Decimal, :to_string, 2), eg: https://github.com/hexpm/hex/blob/3915a9af48c5953d5cc6c871a23c4f2b5a394a3d/lib/hex/version.ex#L241

kinda obscure which exact Decimal module is required

Yes, I should have proposed an optional dep instead, which makes it explicit but doesn't load it.

What do you think? a5af55e

@unti1x
Copy link
Collaborator

unti1x commented Oct 6, 2021

it's unlike that someone is gonna export a module called Decimal

Well, if something may happen, it will. Hopefully, this will save someone's nerves. I still think that having a separate library is better. The other option is to rewrite everything to use some kind of protocol like Stringable or something. But the last word is for @xou

@xou xou force-pushed the master branch 3 times, most recently from e7bdfd0 to 68acd8d Compare January 28, 2024 16:00
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.

None yet

2 participants