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

Shouldn't we handle paging? #188

Open
Nikoloutsos opened this issue Jun 30, 2024 · 4 comments
Open

Shouldn't we handle paging? #188

Nikoloutsos opened this issue Jun 30, 2024 · 4 comments

Comments

@Nikoloutsos
Copy link
Contributor

Nikoloutsos commented Jun 30, 2024

Hello 👋

Recently was in need to fetch some data from GitHub API and I decided to use this library instead of creating my own API calls manually.
Then I realized that there is no easy way to get all contents by handling paging with running the following example.

let issues = try await Octokit(config).issues(owner: "freeCodeCamp", repository: "freeCodeCamp")
// returns 100 issues while it should return 188.

I think it would be nice enhancement for this library to give the option to the developer to choose if he wants to get all content.

What do you say?

@Nikoloutsos
Copy link
Contributor Author

Nikoloutsos commented Jun 30, 2024

Just saw we give the ability to add page on query params. But I think the developer experience would be much better if we do the following:

Usually I am doing kinda something like this to get all contents.

// Do while loop until a page returns no issues.
var issues = [Issue]()
var starting_page = 1
while(true) {
let issuesFromPage =  try await Octokit(config).issues(owner: "freeCodeCamp", repository: "freeCodeCamp", page: starting_page)
issues.append(issuesFromPage)
starting_page += 1
if issuesFromPage.isEmpty { break }
}

instead this could be encapsulated under an enumeration:

issues.append(try await Octokit(config).issues(owner: "freeCodeCamp", repository: "freeCodeCamp", paging_strategy: .allContent))

// or if you want only the first page
issues.append(try await Octokit(config).issues(owner: "freeCodeCamp", repository: "freeCodeCamp", paging_strategy: .page(number: 1, max_items:100)))

The enum would look like this.

enum PagingStrategy {
    case .allContent
    case .page(number: Int, max_items: Int)
}

@pietbrauer
Copy link
Member

I agree that in your context this would make sense. In my opinion the application should handle this though instead of Octokit.swift.
The main reason is that in all other swift contexts (iOS, iPadOS, visionOS, macOS) you might have a table of contents and handle the paging based on the user scrolling the contents.

I am open to discussion here as this seems still possible with your proposed solution. If you are willing you could open a PR and we'll have a look. Ideally this would be handled somewhere central … .

@Nikoloutsos
Copy link
Contributor Author

@pietbrauer I see what you mean. What do you mean by "somewhere central"?

@pietbrauer
Copy link
Member

Central in a way that we have one method that handles the paging in OctoKit.swift so that every API Call method can use it without having to worry which paging option is used.

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

No branches or pull requests

2 participants