xou / elixlsx

An Elixir XLSX writer
MIT License
293 stars 116 forks source link

Add support for Decimal #117

Open leandrocp opened 3 years ago

leandrocp commented 3 years ago

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 (https://github.com/xou/elixlsx/pull/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 commented 3 years ago

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 commented 3 years ago

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? https://github.com/xou/elixlsx/pull/117/commits/a5af55ed6c6ab80e22ce636434094b15862a9e0b

unti1x commented 3 years ago

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