[money-protocol] Expansion: to_string and to_integer
#86
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Also ran the formatter against `money.ex`
pkrawat1
requested changes
Jan 18, 2018
lib/gringotts/gateways/monei.ex
Outdated
| @@ -184,11 +184,12 @@ defmodule Gringotts.Gateways.Monei do | |||
| """ | |||
| @spec authorize(Money.t, CreditCard.t(), keyword) :: {:ok | :error, Response} | |||
| def authorize(amount, card = %CreditCard{}, opts) do | |||
| {currency, value} = Money.to_string amount | |||
There was a problem hiding this comment.
use parens when passing params, its a better way IMO Money.to_string(amount)
lib/gringotts/money.ex
Outdated
|
|
||
| and the API will accept it (as long as the currency is valid [ISO 4217 currency | ||
| code](https://www.iso.org/iso-4217-currency-codes.html)). | ||
| price = %{amount: Decimal.new(20.18), currency: "USD"} |
There was a problem hiding this comment.
What happened to value suggestion for representing amount map. I remember we had a discussion about it internally.
eg:
%{value: Decimal.new(20.18), currency: "USD"}
cc @aviabird/all
lib/gringotts/money.ex
Outdated
| {"USD", 412, -2} | ||
|
|
||
| iex> bhd_price = MoneyLib.new(4.1234, :BHD) | ||
| #MoneyLib<4.1234, "USD"> |
lib/gringotts/money.ex
Outdated
|
|
||
| # the money lib is aliased as "MoneyLib" | ||
|
|
||
| iex> usd_price = MoneyLib.new(4.1234, :USD) |
There was a problem hiding this comment.
I don't think USD is ever dealt like this upto 4 decimal places, typo?
test/integration/money.exs
Outdated
| @ex_money Money.new(42, :EUR) | ||
| @ex_money_long Money.new("42.126456", :EUR) | ||
| @ex_money_bhd Money.new(42, :BHD) | ||
|
|
There was a problem hiding this comment.
We should add test cases to cover Any implementation as well. test cases would help understand our implementation better.
There was a problem hiding this comment.
this test suit has to be more extensive IMO.
* The default rounding strategy for implementations of Gringotts.Money protocol is HALF-EVEN. * Updated public API docs with "perils of rounding".
pkrawat1
pushed a commit
that referenced
this pull request
Jan 25, 2018
* Adds `to_string` and `to_integer` to the protocol * Fixes #62 and #85 * Implements all protocol methods for `ex_money` * Adds integration test with ex_money * Adapted Monei with protocol updates * Adds test for `Any` * The default rounding strategy for implementations of Gringotts.Money protocol is HALF-EVEN. * Updated public API docs with "perils of rounding".
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ex_money(makes use of theCldr.Currencypackage).amountin MONEI!We'll remove the implementations once the lib authors implement it in their
master.