Remove StripeAPIResourceCollection #294
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current implementation has two collection implementations: * `StripeAPIResourceCollection`: Extends `APIResource`; used when a collection needs access to either `request` or `requestCollection`. * `StripeAPICollection`: Used in all other situations. The two collection implementations are functionally identical except for their parent class. This is somewhat error prone because as seen in #292, it's easy to add something to one and forget about the other. This pull eliminates `StripeAPIResourceCollection` and moves all collections over to just extend `StripeCollection`, solving the duplication problem and reducing code complexity. The downside is that I've moved visibility of `request` and `requestCollection` on `APIResource` to public so that they can be called into from collection classes. It would be better if these had something like `internal` visibility (a la C#), but Java doesn't really support anything like that. IMO, it's still a reasonable trade-off and we should still consider those `request*` methods "effectively" private to the package and not part of the public API. Fixes #292.
|
This looks good! Re. visibility, would omitting the modifier altogether (i.e. making the methods package-private) work? |
Good question! So the problem with that one is that the |
|
Ah, I knew I was missing something. I agree that this change is not worth breaking the current namespace structure. Let's 🚢 this! |
|
@olivierbellone Cool! Thanks for taking a look. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current implementation has two collection implementations:
StripeAPIResourceCollection: ExtendsAPIResource; used when acollection needs access to either
requestorrequestCollection.StripeAPICollection: Used in all other situations.The two collection implementations are functionally identical except for
their parent class. This is somewhat error prone because as seen in #292,
it's easy to add something to one and forget about the other.
This pull eliminates
StripeAPIResourceCollectionand moves allcollections over to just extend
StripeCollection, solving theduplication problem and reducing code complexity.
The downside is that I've moved visibility of
requestandrequestCollectiononAPIResourceto public so that they can becalled into from collection classes. It would be better if these had
something like
internalvisibility (a la C#), but Java doesn't reallysupport anything like that. IMO, it's still a reasonable trade-off and
we should still consider those
request*methods "effectively" privateto the package and not part of the public API.
Fixes #292.
/cc @olivierbellone Thoughts on this one? Thanks!