search

mollie / mollie-api-python

Mollie API client for Python
http://www.mollie.com
BSD 2-Clause "Simplified" License
113 stars 55 forks source link

Remove get resource class method #333

Closed Bladieblah closed 10 months ago

Bladieblah commented 1 year ago

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:

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 commented 1 year ago

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 commented 11 months ago

@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 :+1:

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 commented 11 months ago

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 commented 10 months ago

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.