Add params argument to Subscription.delete/2
#386
Conversation
| when params: %{ | ||
| optional(:at_period_end) => boolean | ||
| } | ||
| def delete(id, params \\ %{}, opts \\ []) do |
There was a problem hiding this comment.
@snewcomer Is this a breaking change. If I'm interpreting it correctly, any app that's using this endpoint as
id |> delete(custom_options)will stop working with this change.
Should we add clauses to keep backwards compatibility. For example
def delete(id, opts) when is_list(opts), do: delete(id, %{}, opts)
def delete(id, params) when is_map(params), do: delete(id, params, [])
There was a problem hiding this comment.
@begedin Looking back, it seems params was taken out ~8 months ago and went into 2.0-beta. I missed it in the refactor.
So given the time frame, it's probably a good idea to do what you recommended and perhaps add 2 tests for Subscription.delete - one passing a map and one passing a list as the second arg. 👍
There was a problem hiding this comment.
@begedin You're right! I'd forgotten about backwards compatibility.
What about:
def delete(id, opts \\ []) when is_list(opts), do: delete(id, %{}, opts)
def delete(id, params, opts) when is_map(params) do
# The actual thing
endThis way you can choose to pass params and opts in a call and it's still backwards compatible. The only downside I see is that you can't pass only params and get the default opts.
So, if you want to call the function with params and the default opts it'd have to be:
delete(id, params, [])What do you think? @begedin @snewcomer
There was a problem hiding this comment.
@strattadb I think this will work... Note - without the when params: { spec stuff.
@spec delete(Stripe.id() | t, Stripe.options()) :: {:ok, t} | {:error, Stripe.Error.t()}
def delete(id, opts \\ []) when is_list(opts), do: delete(id, %{}, opts)
@spec delete(Stripe.id() | t, map, Stripe.options()) :: {:ok, t} | {:error, Stripe.Error.t()}
def delete(id, params \\ %{}, opts \\ []) do
There was a problem hiding this comment.
@snewcomer I think that's the best solution, but I tried it in IEx and it didn't compile.
** (CompileError) iex:3: def delete/3 defaults conflicts with delete/2
iex:3: (module)
There was a problem hiding this comment.
@strattadb Oh right b/c of expansion at compile time. I think Nikola was right in the first place 😆 (with guards)...
@spec delete(Stripe.id() | t, Stripe.options()) :: {:ok, t} | {:error, Stripe.Error.t()}
def delete(id, opts) when is_list(opts), do: delete(id, %{}, opts)
@spec delete(Stripe.id() | t, map) :: {:ok, t} | {:error, Stripe.Error.t()}
def delete(id, params) when is_map(params), do: delete(id, params, [])
@spec delete(Stripe.id() | t, map, Stripe.options()) :: {:ok, t} | {:error, Stripe.Error.t()}
def delete(id, params \\ %{}, opts \\ []) do
We basically need an arity of 1, 2, or 3, with the second argument being either of type List or Map.
There was a problem hiding this comment.
@snewcomer Oh, of course! I misunderstood Nikola's suggestion the first time. I'll add the clauses and tests.
Can I ask you why not the params spec clause?
There was a problem hiding this comment.
params spec is needed for the docs. my bad 👌
| optional(:at_period_end) => boolean | ||
| } | ||
| def delete(id, params) when is_map(params), do: delete(id, params, []) | ||
|
|
||
| @doc """ |
There was a problem hiding this comment.
mind moving this doc block above the other two delete funcs?
| params = %{at_period_end: true} | ||
|
|
||
| assert {:ok, %Stripe.Subscription{} = subscription} = | ||
| Stripe.Subscription.delete("sub_123", params) |
There was a problem hiding this comment.
inline params if you wanted while you are in here...
| when params: %{ | ||
| optional(:at_period_end) => boolean | ||
| } | ||
| def delete(id, params) when is_map(params), do: delete(id, params, []) |
There was a problem hiding this comment.
Technically we wouldn't need the third [] since it defaults to that...
There was a problem hiding this comment.
Maybe I'm missing something, but wouldn't that make that clause call itself infinitely?
There was a problem hiding this comment.
I wish I could delete that comment :) . Yep you are right
Fixes #385