Added support for options on the Pay Invoice API #1257
Conversation
| @@ -131,6 +136,15 @@ public virtual async Task<StripeInvoice> PayAsync(string invoiceId, StripeReques | |||
| cancellationToken).ConfigureAwait(false)); | |||
| } | |||
|
|
|||
| public virtual async Task<StripeInvoice> PayAsync(string invoiceId, StripeInvoicePayOptions payOptions, StripeRequestOptions requestOptions = null, CancellationToken cancellationToken = default(CancellationToken)) | |||
There was a problem hiding this comment.
Can you change the first PayAsync method to call this one?
| @@ -151,6 +158,14 @@ public void Pay() | |||
| Assert.Equal("invoice", invoice.Object); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void PayOptions() | |||
There was a problem hiding this comment.
I don't think we need separate tests for both methods, esp. if one simply calls the other. Can you just update the Pay test to add options?
| return this.Pay(invoiceId, null, requestOptions); | ||
| } | ||
|
|
||
| public virtual StripeInvoice Pay(string invoiceId, StripeInvoicePayOptions payOptions, StripeRequestOptions requestOptions = null) |
There was a problem hiding this comment.
I did this to avoid a breaking change but maybe it'd be cleaner to force the option?
| @@ -131,6 +136,15 @@ public virtual async Task<StripeInvoice> PayAsync(string invoiceId, StripeReques | |||
| cancellationToken).ConfigureAwait(false)); | |||
| } | |||
|
|
|||
| public virtual async Task<StripeInvoice> PayAsync(string invoiceId, StripeInvoicePayOptions payOptions, StripeRequestOptions requestOptions = null, CancellationToken cancellationToken = default(CancellationToken)) | |||
There was a problem hiding this comment.
It seems you an not call PayAsync with the option from the one without, it returns a compilation error
Services/Invoices/StripeInvoiceService.cs(132,20): error CS4016: Since this is an async method, the return expression must be of type 'StripeInvoice' rather than 'Task<StripeInvoice>' [/Users/remi/Workspace/bindings/stripe-dotnet/src/Stripe.net/Stripe.net.csproj]
There was a problem hiding this comment.
You want return await PayAsync(...);
There was a problem hiding this comment.
damn I tried a few variations but did not think it just wanted the await. Thanks!
remi-stripe
force-pushed
the
remi-pay-invoice-options
branch
from
84306e9
to
88cfd7e
Compare
|
Fixed PTAL @ob-stripe |
| @@ -123,10 +128,15 @@ public virtual async Task<StripeInvoice> UpdateAsync(string invoiceId, StripeInv | |||
| } | |||
|
|
|||
| public virtual async Task<StripeInvoice> PayAsync(string invoiceId, StripeRequestOptions requestOptions = null, CancellationToken cancellationToken = default(CancellationToken)) | |||
| { | |||
| return await this.PayAsync(invoiceId, null, requestOptions, cancellationToken); | |||
There was a problem hiding this comment.
Actually, I did some research and the right approach here is to not declare this method as async and not use await. Can you update accordingly? Sorry for the extra work :/
There was a problem hiding this comment.
Hmmm, are you sure that method will still appear as Async though and work as expected?
There was a problem hiding this comment.
On third thought, it would work, but it would be a breaking change :/
Let's just get rid of the overloads and release this as a major version?
|
@ob-stripe fixed, PTAL |
remi-stripe
force-pushed
the
remi-pay-invoice-options
branch
from
88cfd7e
to
07e00c6
Compare
|
Released as 18.0.0. |
r? @ob-stripe
cc @stripe/api-libraries