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

Add ListAll methods to resources with ListWithPagination methods #282

Merged
merged 3 commits into from
Apr 21, 2024

Conversation

weirdian2k3
Copy link

This relates to #230

It looks like the List method only returns the first page for all of the resources that we have a ListWithPagination.

I wasn't super comfortable breaking backwards compatibility, so with this PR I'm adding a ListAll method that utilizes ListWithPagination to pull all of the items

Copy link

codecov bot commented Mar 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (3413314) to head (2842ea7).

Additional details and impacted files
@@            Coverage Diff             @@
##            master      #282    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           52        52            
  Lines         2211      2316   +105     
==========================================
+ Hits          2211      2316   +105     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oliver006
Copy link
Collaborator

Thanks for the PR!
I'm a bit slammed right now but will try to get to it in the next few days. Please ping me if you don't hear back from me.

@weirdian2k3
Copy link
Author

@oliver006 no worries. This is this week's ping. I'll hit you up again in a week if I don't hear from you

@oliver006
Copy link
Collaborator

Just catching up here and with #230 so bear with me if I'm a bit slow.

I wasn't super comfortable breaking backwards compatibility

re: backwards compatibility: is your concern that people currently expect List() to return only the first page (or at least not a ton of data) and changing that could cause breakage?

I think that makes sense though I wish we could go back and make the change that List() returns all data (like your ListAll() call) and ListWithPagination could be used for more selective access.

v4.0 is kinda new - do you think we could still make that kind of change and release it as 4.1 ?

@weirdian2k3
Copy link
Author

Just catching up here and with #230 so bear with me if I'm a bit slow.

I wasn't super comfortable breaking backwards compatibility

re: backwards compatibility: is your concern that people currently expect List() to return only the first page (or at least not a ton of data) and changing that could cause breakage?

Exactly, I don't want to be the reason someone was downloading 50 items and is now downloading 5,000 just by updating a dependency

v4.0 is kinda new - do you think we could still make that kind of change and release it as 4.1 ?

We can, but I'm worried that could be confusing since the pagination was only added to some of the endpoints, so List might still return just some for the other entities

I think the easiest option is to go with this for now, then go through and see what other entities now support pagination (if there have been any additions) and if there is now pagination for all endpoints, introduce that as a breaking change in maybe a 4.2

@oliver006
Copy link
Collaborator

I think what you say makes sense, let's keep ListAll()

Copy link
Collaborator

@oliver006 oliver006 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

@@ -111,6 +112,29 @@ func (s *CustomerServiceOp) List(ctx context.Context, options interface{}) ([]Cu
return resource.Customers, err
}

// ListAll Lists all customers, iterating over pages
func (s *CustomerServiceOp) ListAll(ctx context.Context, options interface{}) ([]Customer, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry you had to write this 15 times.

One day I'll get around to redo a large part of this library using generics but today is not that day
(plus it'll break backwards compatibility with slightly older Golang versions)

@oliver006 oliver006 merged commit 3554221 into bold-commerce:master Apr 21, 2024
5 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.

2 participants