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

Pagination utilities #44

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Pagination utilities #44

wants to merge 5 commits into from

Conversation

craigtweedy
Copy link
Contributor

@craigtweedy craigtweedy commented Jan 23, 2019

Status

  • 🚧 WIP

Feature

Implements a new feature

Description

Adds some utility functions to the PaginatedResponse class which allows for simpler iteration through pages.

moltin.product.all { (result) in
    guard case .success(let firstPage) = result else { return }
    
    print(firstPage)
    var currentPage = firstPage
    
    while currentPage.links?["next"] != nil {
        currentPage.next(withConfig: moltin.config) { (paginatedResult) in
            guard case .success(let nextPage) = paginatedResult else { return }
            currentPage = nextPage
            print(currentPage)
        }
    }
}

Notes

Ideally, I'd not want the configuration to be specified again for each function, however, the response class does not have any understanding of the request that made it, so that would have to change and I'm not sure if I want that right now.

@craigtweedy
Copy link
Contributor Author

@gje4 Any comments on the way it works? Thoughts about how to improve it so it doesn't require the config?

@gje4
Copy link
Contributor

gje4 commented Jan 28, 2019

Spent some more time with this. First thought would be a move to setting config to a global level. Either requiring the app to be set once at the appDelegate or even in the pList. It does have some restrictions that we have talked about in the past.

The other thought not sure if its worth exploring/thoughts.
Move the next functionality to the Moltin class
Something like
public func next(forType object: , completionHandler: @escaping moltin.ProductRequest.DefaultCollectionRequestHandler) -> moltin.MoltinRequest

Or even tell them to pass the link
public func next(nextData link: String, completionHandler: @escaping moltin.ProductRequest.DefaultCollectionRequestHandler) -> moltin.MoltinRequest

Think that may not be very efficient though.

I guess in closing, I do not think it is too much to ask for config. I also think from a practical stand point needing that much data for the UI is going to be the edge case. So probably not worth to much time.

@craigtweedy
Copy link
Contributor Author

I'm pretty against making this config global - would prefer to keep everything contained to the Moltin module. It's on the implementing developer to then decide how best to manage both the class and the configuration.

The functions really already take the URL from the links dictionary, config is largely needed in order to construct other parameters such as authentication headers.

@gje4
Copy link
Contributor

gje4 commented Jan 28, 2019

All good with me, think it adds value and should be pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants