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 pagination helper to easily collect all items #1498

Closed
wants to merge 1 commit into from

Conversation

timofurrer
Copy link
Collaborator

🚧 THIS IS MERELY AN EXPERIMENT FOR NOW! 🚧

The #1496 Pull Request triggered
some thinking for how to do this in a more "generic" way.

Thus, I drafted this gitlab.Collect() function, which can be used to
"easily" collect all items of a specific "list" API (I've implement a
collect function which eagerly collects all items, but something
similar can be implemented for channels to stream).

The only downside of the current implementation so far is that Go
doesn't really support generics / interfaces with direct field access as
of now ... effectively, you'll always need methods.
This means that, because the ListOptions field is somewhat
inconsistently (sometimes as subfield, sometimes it's promoted)
implemented over this package every gitlab.List*Options struct needs
it's own GetPage() int and SetPage(int) implementation ...

One improvement for this could be that we split out the ListOptions
from the API endpoint specific options and make it an additional
argument to the list methods - so that Collect() could profit from
this and GetPage() int and SetPage(int) would only need to be
implemented once.

Another one would be to just use gitlab.ListOptions for endpoints
which don't have additional parameters instead of doing this type ListInstanceVariablesOptions ListOptions.

However, I think best would be to separate the generic ListOptions to
a separate argument in the long term ...

@svanharmelen what do you think?

The xanzy#1496 Pull Request triggered
some thinking for how to do this in a more "generic" way.

Thus, I drafted this `gitlab.Collect()` function, which can be used to
"easily" collect all items of a specific "list" API (I've implement a
*collect* function which eagerly collects all items, but something
similar can be implemented for channels to stream).

The only downside of the current implementation so far is that Go
doesn't really support generics / interfaces with direct field access as
of now ... effectively, you'll always need methods.
This means that, because the `ListOptions` field is somewhat
inconsistently (sometimes as subfield, sometimes it's promoted)
implemented over this package every `gitlab.List*Options` struct needs
it's own `GetPage() int` and `SetPage(int)` implementation ...

One improvement for this could be that we split out the `ListOptions`
from the API endpoint specific options and make it an additional
argument to the list methods - so that `Collect()` could profit from
this and `GetPage() int` and `SetPage(int)` would only need to be
implemented once.

Another one would be to just use `gitlab.ListOptions` for endpoints
which don't have additional parameters instead of doing this `type
ListInstanceVariablesOptions ListOptions`.

However, I think best would be to separate the generic `ListOptions` to
a separate argument in the long term ...
@2785
Copy link
Contributor

2785 commented Nov 18, 2022

A while back I actually wrote a flavor of this, driven by generics & gammazero's deque that vaguely works like this

func NewProjectMRIterator(ctx context.Context, pid string, opts gitlab.ListProjectMergeRequestsOptions) *ListItemIterator[*gitlab.MergeRequest] {
	return NewListItemIterator(ctx, func(ctx context.Context, page int) (results []*gitlab.MergeRequest, nextPage int, err error) {
		glClient, err := GetGlobalGitlabClient()
		if err != nil {
			return nil, 0, err
		}

		opts.Page = page

		mrs, resp, err := glClient.MergeRequests.ListProjectMergeRequests(pid, &opts, gitlab.WithContext(ctx))
		if err != nil {
			return nil, 0, err
		}

		return mrs, resp.NextPage, nil
	})
}

(if you ignore the global client business)

It's basically

type Iterator[T any] interface {
	Next() (T, error)
}

then you can have your usual DrainIterator[T any](i Iterator[T]) ([]T, error) style utils all you want.

but backed by a queue & goes to fetch a new batch with a user supplied function, when the queue is empty.

Can put a PR up if yall fancy.

@svanharmelen
Copy link
Member

I think I'm going for PR #1875 for adding a form of pagination support. If you think that solution can or should be changed/updated (including naming or ergonomics) than please comment on that PR so we can get to some kind of consensus before merging that one 😏

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