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

Remove get resource class method #333

Merged

Conversation

Bladieblah
Copy link
Contributor

@Bladieblah Bladieblah commented Aug 17, 2023

I have refactored the ObjectList class in order to solve #315 with minimal changes. I completely removed the get_resource_class method from all the Resources, which now instead just store the type of the underlying object. The ObjectList class was being used for (as far as I'm concerned) 2 purposes:

  • Paginating list endpoints
  • Extracting embedded child objects from their parent

This distinction in function is now explicit with the PaginationList and ObjectList classes. Another benefit of this approach, is that paginating an endpoint no longer creates duplicate Resource objects.

Let me know what you think!

@whyscream
Copy link
Contributor

Hi @Bladieblah, thanks for your PR. Due to holidays, we were unable to give you a response any sooner than today. We'll look into your work later this week.

@whyscream
Copy link
Contributor

whyscream commented Sep 11, 2023

@Bladieblah Your idea has an interesting, different take on the problem than we already tried in #323.

You state that your PR solves the issue with minimal changes. The actual problem with the pagination is that none of it is under test, which is why we never discovered and solved the issue in the first place. Your PR unfortunately doesn't fix that, and we have a different PR in draft that also tries to solve this issue in a different way, with some tests added (but not completed yet).

I took your branch and added a few tests on Subscriptions (the original issue) and CustomerSubscriptions (a nested variant of the same endpoint), they pass so your code seems to be okay 👍

Note: your PR mentions removal of get_resource_class in several places, but in fact you removed get_resource_object. The get_resource_class method has its own problems, but you're not fixing them here (which is fine).

@Bladieblah
Copy link
Contributor Author

Hi @whyscream, thanks for looking at the PR!

I messed up somewhere because I definitely intended to remove get_resource_class everywhere, I made the changes first in another repo for an API client that I based off of this one. I now made the remaining changes so get_resource_class is completely gone. I also added the tests you made for the pagination endpoint and they do indeed pass.

Apologies for the confusion!

@whyscream whyscream force-pushed the remove-get-resource-class-method branch 3 times, most recently from 956a2d0 to 31a3f46 Compare October 12, 2023 11:41
@whyscream
Copy link
Contributor

I added all tests I wrote for Subscriptions, and also some new tests for all places where you can list Chargebacks. I think we covered not all, but at least some of the places where pagination happens, so that we can verify that it works now.

@whyscream whyscream force-pushed the remove-get-resource-class-method branch from 31a3f46 to 547bffe Compare October 13, 2023 11:05
@geertjanvdenbosch geertjanvdenbosch merged commit a628ab0 into mollie:master Oct 13, 2023
9 checks passed
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

Successfully merging this pull request may close these issues.

3 participants