Add specialized ListOptions for each resource #1083
Conversation
| @@ -54,7 +54,7 @@ public transfer_reversals_fixture() | |||
| TransferReversal = service.Create(Transfer.Id, TransferReversalCreateOptions); | |||
| TransferReversalUpdated = service.Update(Transfer.Id, TransferReversal.Id, TransferReversalUpdateOptions); | |||
| TransferReversalRetrieved = service.Get(Transfer.Id, TransferReversal.Id); | |||
| TransferReversalList = service.List(Transfer.Id, new StripeListOptions()); | |||
| TransferReversalList = service.List(Transfer.Id, TransferReversalListOptions); | |||
There was a problem hiding this comment.
why don't you need a new here?
There was a problem hiding this comment.
Oops! I didn't see that TransferReversalListOptions was declared but never initialized. This is now fixed.
| @@ -17,7 +17,7 @@ public class when_listing_country_specs | |||
| Because of = () => | |||
| { | |||
| _countrySpecList = _countrySpecService.List( | |||
| new StripeListOptions() { Limit = 8 } | |||
| new CountrySpecListOptions() { Limit = 8 } | |||
There was a problem hiding this comment.
why not StripeCountrySpecListOptions?
There was a problem hiding this comment.
To maintain consistency with all the other classes (service and entities). I agree that we should fix this, but it's a job for a different PR and in the meantime I'd rather maintain consistency within each resource.
|
|
||
| namespace Stripe | ||
| { | ||
| public class BankAccountListOptions : StripeListOptions |
There was a problem hiding this comment.
Why not StripeBankAccountListOptions? I guess it's because of the class name discrepancies but it makes it confusing
There was a problem hiding this comment.
Same answer as for CountrySpec: to maintain internal consistency with the other BankAccount classes (entity, service and other options classes). Also something that we should fix separately.
There was a problem hiding this comment.
I see but it seems better (to me) to keep consistency with the new naming of the ListOptions than the parent class itself. Especially as those are the only 2 that are different
There was a problem hiding this comment.
I disagree, but since we're going to release a new major version anyway, I guess now's as good a time as any to fix all those classes :)
r? @remi-stripe
cc @stripe/api-libraries @anelder-stripe
Fixes #974.
This PR adds specialized ListOptions classes for every resource.