Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CreditNoteService should implemented INestedListable? #2022

Open
clement911 opened this issue May 4, 2020 · 6 comments
Open

CreditNoteService should implemented INestedListable? #2022

clement911 opened this issue May 4, 2020 · 6 comments

Comments

@clement911
Copy link
Contributor

clement911 commented May 4, 2020

CreditNoteService can list its nested lines in a separate endpoint, currently implemented with ListLineItems().
To be consistent with the other nested services, shouldn't it implement INestedListable?
It would allow for generic code to handle nested pages consistently.

I guess CreditNoteService is unique in the sense that it is used to list both credit notes and credit note lines. That seems to be just a consequence of how the docs are structured. I guess a separate CreditNoteLineService could make sense also?

@clement911
Copy link
Contributor Author

And one can make the exact same argument for InvoiceService.ListLineItems.
As far as I can see these are the only 2 occurrences.

@ob-stripe
Copy link
Contributor

Yes, invoice line items and credit note line items are technically resources nested under invoices and credit notes respectively, so if we wanted to be fully consistent with other nested resources are handled, we should introduce new InvoiceLineItemService and CreditNoteLineItemService service classes, and replace InvoiceItemService.ListLineItems with InvoiceLineItemService.List and CreditNoteService.LineLineItems with CreditNoteLineItemService.List.

If we did that, the two new services could implement INestedListable, but we can't add it to the existing InvoiceService and CreditNoteService classes because the interface wouldn't match (specifically, the method names are different: List in the interface vs. ListLineItems in the service classes).

We could change it, but I'm not sure it's worth the hassle. Also, having InvoiceService, InvoiceItemService and InvoiceLineItemService would be pretty confusing.

We're in the process of making changes to how line items as a whole are handled in the API, so we'll probably wait until those changes land in the backend then make a decision regarding the .NET API.

@clement911
Copy link
Contributor Author

Ok, thank you for your comments.
I must say I'm pretty amazed at how quickly the API is changing. Or maybe I should say, augmenting.

@clement911
Copy link
Contributor Author

Note that the SessionService suffers from the same issue. It lists its line with ListLineItems() instead of using an INestedListable service which would help with generic code.

@pakrym-stripe
Copy link
Contributor

@clement911, can you share an example of how you are using the INestedListable interface? We were debating the usefulness of them internally and seeing an actual use-case will help.

@clement911
Copy link
Contributor Author

Yes we have a big dependency on these interfaces and related types, especially IListable, IRetrievable, INestedListable, INestedRetrievable, but also ListOptions, BaseOptions, IStripeEntity, IHasId, IHasOjbect.

We leverage these common interfaces to create reusable code that can work with any of the Stripe services to load various objects.

In particular, we have several bases classes to handle the different data fetching patterns exposed by the Stripe API.

Here are a few snippets of code from the bases classes, each of them have up to ten concrete implementations that inherit from them.

image

image

image

image

Hopefully this gives you an idea of how much we use these common types and how they are really simplifying our code and making it reusable.

We did find a few instances where the .NET API doesn't follow these interface conventions, and in those instances we've had to write custom adapter classes to make them conform. Obviously, we'd prefer it if the interfaces were implemented already by the Stripe.NET library.

CreditNoteService is one such example where we would prefer if there was a CreditNoteLineService implementing ServiceNested. Similarly, for consistency we think there should be a InvoiceLineService and SessionLineService that should implement ServiceNested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants