Cleanup StripeCard - v2.0.0-beta.1 #342
Conversation
You are correct on what it is. As for what it should contain, it should follow the stripe API documentation for that action on that endpoint.
The documentation for this is in |
lib/stripe/payment_methods/card.ex
Outdated
| @spec create(params, Keyword.t()) :: {:ok, t} | {:error, Stripe.Error.t()} | ||
| when params: %{ | ||
| optional(:customer) => Stripe.id() | Stripe.Account.t() | ||
| } |
There was a problem hiding this comment.
@begedin @joshsmith I guess what I was wondering is what keys should be included here. I see in other module that it's only a small subset. Also does clean up the params refer to only this spot in the guard clause? Or is there other spots this request is asking for?
There was a problem hiding this comment.
For example, this is only a limited list of the Subscription struct's keys...
There was a problem hiding this comment.
In the example of a subscription, if you take a look at the documentation for the create action
https://stripe.com/docs/api#create_subscription
you will see that the spec matches that list much better (though we're still missing prorate as an optional boolean).
So to conclude, each action's @spec should match the arguments list for that action in stripe's documentation.
snewcomer
changed the title
|
@begedin This is ready for your review! |
| metadata: Stripe.Types.metadata() | nil, | ||
| metadata: %{ | ||
| optional(String.t()) => String.t() | ||
| } | nil, |
There was a problem hiding this comment.
What's the reasoning for replacing this? I'm not saying it's wrong and I vaguely recall a conversation with someone about it, but it looks like Stripe.Types.metadata is defined as exactly this here, so it sounds reasonable to leave it as is.
There was a problem hiding this comment.
I looked at the coupon PR. But now looking in types.ex, this is the exact same thing. Will revert 👍
lib/stripe/payment_methods/card.ex
Outdated
| metadata: Stripe.Types.metadata(), | ||
| metadata: %{ | ||
| optional(String.t()) => String.t() | ||
| }, |
There was a problem hiding this comment.
See comment above. Again, sorry if there's a conversation about this somewhere which I forgot about.
|
@begedin Fixed. Replaced all with usages with https://github.com/code-corps/stripity_stripe/blob/2.0-beta/lib/stripe/types.ex#L23 |
snewcomer
force-pushed
the
sn/2.0-beta/stripe-card
branch
2 times, most recently
from
d2c066b
to
f466a46
Compare
snewcomer
force-pushed
the
sn/2.0-beta/stripe-card
branch
from
f466a46
to
0d9126a
Compare
|
Good woork as always @snewcomer. Much appreciated 👍 |
@begedin
https://stripe.com/docs/api#create_subscription
Just getting started and have a few questions:
Clean up the params.mean the@specparams? If so, what is the background on why and what is the scope of keys that should be included?Add from_jsonmacros - a bit confused. Followed this back tostripe/converter.ex. Is this already implemented for all modules given requests are all passed throughmake_request(which eventually callsConverter.convert_result(result)?Progress on #266