Custom method signature requirements

[joeltaylor]

Summary

I've noticed that there are some unexpected differences between CRUDL and the custom method signatures.

For example, I commonly perform operations with the following signature:

Stripe::PaymentIntent.create(hash, 'api_key_override')

Which usually leads me to try something like:

Stripe::PaymentIntent.cancel('pi_1234', 'api_key_override')
# Results in an error: NoMethodError: undefined method `key?' for "sk_test"

Because what it really wants is:

Stripe::PaymentIntent.cancel('pi_1234', {}, 'api_key_override')

After a bit of thrashing, I usually remember the signature difference, but it strikes me as odd that params is in the signature of the signature for some methods that don't accept them (e.g. canceling a PaymentIntent doesn't accept any).

Thoughts/ideas

• Is there any interest in establishing a consistent convention for the method signature across all API operations? I saw in one discussion that the .retrieve method signature may be deprecated so I'm not sure if the signature I've been using is on the chopping block.
  • Tangentially, I wonder if keyword arguments would make the differences less surprising
• If the long-term plan is to accept the nuance differences, perhaps a better error message would do the trick.

I'm happy to help out and open a PR!

[ob-stripe]

Hi @joeltaylor, thanks for the feedback.

I think there are two kinds of API operations that need to be distinguished:

• those that operate directly on a collection (create, list)
• those that operate on a specific element in a collection (retrieve, update, delete, and custom methods)

The latter need a resource ID, the former don't. So the signatures need to be different:

• create and list operations should have a signature like method(params, opts)
• retrieve, update, delete and custom methods should have a signature like method(id, params, opts)

Unfortunately, for historical reasons the signature of retrieve is different. It's retrieve(id, opts), but you can also pass a Hash for the id parameter and the library will extract the id key to use as the resource ID, and all other keys to use as regular parameters. See #809 for more context (but I assume you've already seen this and it's the discussion you mentioned).

I could be wrong but I think retrieve is the only method with an inconsistent signature, and all other API operations follow the standard pattern I explained above.

We should probably fix retrieve to use the same pattern, but it would be a breaking change and would have to wait until the next major version.

[joeltaylor]

Thanks for the rundown @ob-stripe!

I think that all makes sense given the two kinds of API operations.

What do you think about an improved error message for when an api key is detected within the params portion of the signature?

This was the error message I found confusing:

Stripe::PaymentIntent.cancel('pi_1234', 'api_key_override')
# Results in an error: NoMethodError: undefined method `key?' for "sk_test"

I may be an outlier with my use case, but would be happy to take a stab at a PR or close the issue if it's superfluous.

[ob-stripe]

Right, we can definitely improve the error message. I'll be happy to review a PR if you want to take a stab at it!