Fix signatures in InvoiceService and RefundService
#1287
Conversation
| @@ -7,17 +7,33 @@ | |||
|
|
|||
| public class InvoiceCreateOptions : BaseOptions, ISupportMetadata | |||
| { | |||
| /// <summary> | |||
| /// A fee in cents that will be applied to the invoice and transferred to the application | |||
| /// owner’s Stripe account. The request must be made with an OAuth key or the Stripe-Account | |||
There was a problem hiding this comment.
I don't like this comment at all. It mentions the OAuth key that we really don't want anyone to use in general. I know it's copied from the docs verbatim but I usually only take the first sentence if it has enough meaning.
| @@ -23,38 +23,38 @@ public InvoiceService(string apiKey) | |||
|
|
|||
| public bool ExpandSubscription { get; set; } | |||
|
|
|||
| public virtual Invoice Get(string invoiceId, RequestOptions requestOptions = null) | |||
| public virtual Invoice Create(InvoiceCreateOptions createOptions = null, RequestOptions requestOptions = null) | |||
There was a problem hiding this comment.
I don't understand why the options are optional. I guess it makes it easier for the service later but since the vast majority of create methods have required parameters it "feels" wrong.
| /// </summary> | ||
| [JsonProperty("billing")] | ||
| public Billing? Billing { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The number of days from which the invoice is created until it is due. Only valid for invoices where billing=send_invoice. | ||
| /// REQUIRED |
There was a problem hiding this comment.
Not a fan of the REQUIRED part either
| Requestor.GetString( | ||
| this.ApplyAllParameters(null, $"{Urls.Invoices}/{invoiceId}", false), | ||
| Requestor.PostString( | ||
| this.ApplyAllParameters(createOptions, Urls.Invoices, false), |
There was a problem hiding this comment.
Flagging for later, can we find a way to have the URL be an a class method/instance instead while we do that work of porting to the new services logic?
There was a problem hiding this comment.
well noticing you did it for refund below
ob-stripe
force-pushed
the
ob-fix-signatures
branch
from
e05063f to
f91a21c
Compare
That's not going to happen anytime soon though, and in the meantime I think that having some comments is strictly better than having no comments at all. |
r? @remi-stripe
cc @stripe/api-libraries
Fixes signatures in
InvoiceServiceandRefundServiceso that required parameters are passed in options classes rather than as their own arguments.Also a bunch of cleanup:
UpcomingInvoiceOptions