kipcole9 / money_sql

Money functions for the serialization of a money data type in Elixir
Other
28 stars 18 forks source link

Helpers for querying DB #29

Closed am-kantox closed 1 year ago

am-kantox commented 1 year ago

First of all, let me thank you for ex_cldr, ex_money, and for the hugely valuable contribution to Elixir ecosystem in general.

This PR has a low value but still might be helpful for newcomers to better express their intent. fragment/1 is fragile in many ways, so helpers like these might simplify the queries and make them more readable.

Organization
|> where([o], money_is(o.payroll, Money.new!(100, :USD)))
|> select([o], o.payroll)
|> Repo.all()
#⇒ [Money.new(:USD, "100"), Money.new(:USD, "100")]

Organization
|> where([o], amount_ge(o.payroll, 90))
|> select([o], o.payroll)
|> Repo.all()
#⇒ [Money.new(:AUD, "90"), Money.new(:USD, "100")]

Organization
|> where([o], o.name == ^"Lemon Inc.")
|> total_by([o], o.payroll, :USD)
|> Repo.one()
#⇒ [Money.new(:USD, "210")]

The architecture allows easy implementation of MySQL adapter (via JSON_EXTRACT,) please let me know if you are interested in adding all that to the library.

Thanks again, and have a nice day.

kipcole9 commented 1 year ago

@am-kantox, thank you, that's much appreciated and a very good idea. I would prefer to have symmetry between the two types if at all possible (composite and map). Possible? And I think money_is would be more appropriately named money_eq to be more consistent with Elixirs :eq, :lt, :gt symbols.

kipcole9 commented 1 year ago

And thank you for the kind words, its appreciated and it helps keep my motivation going!

am-kantox commented 1 year ago

I would prefer to have symmetry between the two types if at all possible (composite and map). Possible?

Yes, and no. As I said, it’s possible to implement the adapter for MySQL and I am all in into implementing it. As it is now, it still allows you to plug in the home-baked adapter with use Money.Ecto.Query.API, adapter: HomeBaked.MySQL.Adapter, that’s why Money.Ecto.Query.API declares a behaviour in the first place.

But in general, we cannot implement it “for map,” because it heavily relies on how the map is mapped (pun unintended) into the database. For mongo, or khepri, we’ll need to [most likely] introduce another adapter.

I can add MySQL implementation into this PR, or we might split it into two. It would take a while because the last time I dealt with MySQL was in high school, but it should not take too long.

And I think money_is would be more appropriately named money_eq

🤦🏼 of course, stay tuned.

am-kantox commented 1 year ago

That was fun! I have not done as much SQL for decades.

Now we have three implementations of API,

The former is what I committed today morning. Two having Map in the name are for MySQL JSON and Postgres JSONB respectively. The interface stays the same.


I have tested MySQL implementation, and there is a test for it with @moduletag skip: true because proper testing against both databases would have increased the size of this PR twice.

Hope it works for you.

kipcole9 commented 1 year ago

This is a amazing @am-kantox, really appreciated. I'll review after my morning coffee and aim to merge today and unless there are any unexpected issues we should be able to publish this weekend.

am-kantox commented 1 year ago

There is no rush, also, if you have a different vision of elegance in some approaches, please don’t hesitate to simply update the code; at the end of the day, it’s your library and it’s better to be consistent.

I care about the result and I absolutely do not care about my code being thrown out to the trash cam, I am not [unfortunately] in my twenties anymore :)

kipcole9 commented 1 year ago

Looks to me like its in good shape to merge? I can slipstream this in to the CLDR 43-based updates for many libs this week.

am-kantox commented 1 year ago

Sounds great!

Let’s merge it and slipstream into CLDR 43.