Skip to content

Remove get resource class method#333

Merged
geertjanvdenbosch merged 11 commits intomollie:masterfrom
Bladieblah:remove-get-resource-class-method
Oct 13, 2023
Merged

Remove get resource class method#333
geertjanvdenbosch merged 11 commits intomollie:masterfrom
Bladieblah:remove-get-resource-class-method

Conversation

@Bladieblah
Copy link
Copy Markdown
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!

Comment thread mollie/api/objects/list.py
Comment thread mollie/api/objects/list.py
Comment thread mollie/api/objects/list.py
Comment thread mollie/api/resources/methods.py
@whyscream
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread tests/test_list.py Outdated
Comment thread tests/test_list.py Outdated
@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
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