kipcole9 / money

Elixir implementation of Money with Currency
https://hex.pm/packages/ex_money
Other
572 stars 48 forks source link

Evolve The Inspect Protocol Implementation #132

Closed Adzz closed 2 years ago

Adzz commented 3 years ago

Currently when we log a Money struct to the console it looks like this:

#Money<:GBP, 12.20>

This is good but it is not valid elixir syntax which makes copying it to an iex shell (for example) problematic.

One possible alternative is to do like Date does and print a sigil out:

IO.inspect(Date.utc_today())
#=> ~D[2021-09-01]

IO.inspect(Money.new(:GBP, "12.20"))
#=> ~M[12.20]gbp

OR

I recenlty opened a similar PR on Decimal which changes the output to beDecimal.new we could do similar here:

IO.inspect(Money.new(:GBP, "12.20"))
#=> Money.new(:GBP, "12.20")

What do you think?

coladarci commented 3 years ago

Personally, I love this... Dumping a sigil would require you to import that sigil before using, so I MIGHT prefer your second suggestion, though it did strike me as a little funny at first...

Curious what @kipcole9 thinks about this!

kipcole9 commented 3 years ago

its an interesting idea for sure. Three thoughts come to mind:

  1. Does this break the intent of the Inspect protocol? I recognise that Date and friends output a sigil_* but for maps and structs in general thats not the case.
  2. Does outputting valid code from Inspect introduce an unreasonable cognitive load. ie you're sitting at iex prompt and the return result from some function is more code. How does your brain work out when you're looking at an inspected result versus something you were typing? After all, v() in iex returns the result of the last function (and v(n) returns the nth iex value) so that may not be enough motivation.
  3. Does this make doctests less understandable? Especially for newcomers to the library and potentially also new to Elixir. Is this a practise in common use in other languages? And does this make ex_money too different to community expectations?

I think I will post this idea into Elixir Forum and see if there is a community view to add to the discussion.

Adzz commented 3 years ago

Great points! I'm interested to see what the forum says too.

My rough thoughts are:

  1. Tricky to say, but I think it's okay - as Date / DateTime etc do the same. I think the value of having the output be executable is worth it in this case...
  2. Again hard to say, but the output is a different colour in IEX at least: image

Does this make doctests less understandable

I don't think so, and I think being able to copy / paste doc tests into an actual repl could possibly help doctests.

I think if Decimal go for it it'll be an interesting precedent!

Adzz commented 2 years ago

Hey 👋 just to update Decimal merged the PR I made to do the same thing over there. Also, Elixir is planning to do the same for some of the core data types.

kipcole9 commented 2 years ago

That's great - will move forward with this now, and will do the same for ex_cldr_units. Might take me a few days given some other priorities but I'm on it!

Adzz commented 2 years ago

Thanks Kip! 🙇 Do shout if you would like a PR to help.

kipcole9 commented 2 years ago

As of commit daf0867 the inspect protocol implementation is updated and doc tests changed to reflect that change. All tests and dialyzer are happy.

There is one issue I am aware of - some doc tests assume the "old" inspect implementation of Decimal so I need to adjust those doc tests before release.

If you have a chance to run {:ex_money, github: "kipcole9/money"} that would be great!

kipcole9 commented 2 years ago

I've updated the doc tests that return Decimal results to a format that should work on all Decimal releases.

LostKobrakai commented 2 years ago

Given this affects dcotest would this be a breaking change?

kipcole9 commented 2 years ago

I'm going to see if Elixir and Decimal consider it a breaking change. I suspect not but I'll follow their lead.

kipcole9 commented 2 years ago

Sorry for not bringing this to resolution sooner but I implemented expression based inspect in this commit and published the update in ex_money 5.9.0 on Feb 12, 2022.

Hopefully the changes meet your expectations? (or maybe you didn't notice :-))

Adzz commented 2 years ago

It was great thanks!