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

feat: add AllPages pagination helper #1875

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

costela
Copy link
Contributor

@costela costela commented Feb 5, 2024

This small PR finally addresses #1320, picking up where the discussion left off.

Naming is - of course - up for discussion! :)

Update: the PR now also includes an experimental iterator based on the rangefunc experiment.

@costela costela force-pushed the add-all-pages-pagination-helper branch from 0d4604e to 9067982 Compare February 5, 2024 22:11
@costela
Copy link
Contributor Author

costela commented Feb 7, 2024

Oh, this is embarrassing, I totally missed #1741 when searching for previous work 😕

This implementation is considerably different though, so I'll leave the PR open for now.

@costela costela mentioned this pull request Feb 7, 2024
@Roemer
Copy link

Roemer commented Feb 7, 2024

Looks cool but what I am missing from my PR (#1741) is the ability to 1. somehow allow the pagination to abort on some criteria and 2. the ability to immediately process each result. Both are a bit of edge cases but can come in handy in a few situations.

@costela
Copy link
Contributor Author

costela commented Feb 7, 2024

I guess the iterator approach basically solves both your points?
We could reduce this PR to just the iterator; just for those who explicitly want to test the new rangefunc stuff?

@Roemer
Copy link

Roemer commented Feb 7, 2024

Ah true, I misread that. Seems that the iterator approach could really do those things. That would be fantastic and way less clunky than my previous version.

@costela
Copy link
Contributor Author

costela commented Feb 7, 2024

Just for the record: note that the iterator doesn't technically require the rangefunc experiment or go1.22, but I think we should still "hide" it behind the experiment to limit API breakage to those explicity interested.

If go1.23 turns out to break the rangefunc experiment API, we can then change the iterator with minimal user impact.

@JordanP
Copy link
Contributor

JordanP commented Feb 7, 2024

It took me at least 5min to understand this line func PageIterator[O, T any](f Paginatable[O, T], opt *O) iter.Seq2[*T, error] { but it's beautiful !

@costela
Copy link
Contributor Author

costela commented Feb 7, 2024

Just 5min? Then you were faster than me! 😅

Seriously though, I think renaming the type parameters to something more verbose may help?

And I think as long as the usage is straightforward (which IMHO it still is), then the implementation is allowed to look a bit gnarly.

As a side-note, I'm personally still a bit torn on the rangefunc stuff. Powerful and elegant (at least from a runtime perspective) but twists my brain in ways I'm not yet comfortable with.

@JordanP
Copy link
Contributor

JordanP commented Feb 9, 2024

As a side-note, I'm personally still a bit torn on the rangefunc stuff. Powerful and elegant (at least from a runtime perspective) but twists my brain in ways I'm not yet comfortable with.

Yep ! For me, that feeling started with the introduction of generics. As a "user"/consumer, they are a joy to use. As a code author/maintainer, they are a pain.


Some functions take an additional pid interface{} as first argument (like Pipelines.ListProjectPipelines(...)). So I need a function like

func XXYYY[O, T any](pid interface{}, f func(interface{}, *O, ...gitlab.RequestOptionFunc) ([]*T, *gitlab.Response, error)) Paginatable[O, T] {
	return func(opt *O, optionFunc ...gitlab.RequestOptionFunc) ([]*T, *gitlab.Response, error) {
		return f(pid, opt, optionFunc...)
	}
}

It's a function that returns a closure, whose signature matches type Paginatable[O, T any]

Would that make sense to include such function in that gitlab package ?Is there a better way ?

iterator.go Outdated
return func(yield func(*T, error) bool) {
nextLink := ""
for {
page, resp, err := f(opt, WithKeysetPaginationParameters(nextLink))
Copy link
Contributor

@JordanP JordanP Feb 9, 2024

Choose a reason for hiding this comment

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

The function f accepts a variable number of RequestOptionFunc arguments, but in this call here, it passes a single one.

Copy link
Contributor

@JordanP JordanP Feb 9, 2024

Choose a reason for hiding this comment

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

Maybe something like that

//	for user, err := range gitlab.PageIterator(gl.Users.List, nil, gitlab.WithContext(ctx)) {
//		if err != nil {
//			// handle error
//		}
//		// process individual user
//	}
func PageIterator[O, T any](f Paginatable[O, T], opt *O, optionFunc ...gitlab.RequestOptionFunc) iter.Seq2[*T, error] {
	return func(yield func(*T, error) bool) {
		nextLink := ""
		for {
			page, resp, err := f(opt, append(optionFunc, gitlab.WithKeysetPaginationParameters(nextLink))...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, good point! Thankfully a quick fix. PTAL

@costela costela force-pushed the add-all-pages-pagination-helper branch 2 times, most recently from cb6dd4c to fb83965 Compare February 14, 2024 22:06
@costela
Copy link
Contributor Author

costela commented Feb 14, 2024

It's a function that returns a closure, whose signature matches type Paginatable[O, T any]

Would that make sense to include such function in that gitlab package ?Is there a better way ?

Also a good point. However, I'm not sure about the closure approach. It's elegant, but I fear it may not be directly clear for consumers.
I've added an alternative suggestion to the code: *ForID variants to the pagination helpers. IMHO that's slightly more ergonomic for users, but I'll gladly refactor into the closure approach if that's the preferred way.

The two versions unfortunately only cover around 60% of list-like methods: if my grep-foo is correct, there are currently 187 list-like methods in total, 25 of the "simple" opt+optFunc type and 85 of the id+opt+optFunc type.
Hopefully that's still enough of an improvement to warrant including it?

@costela
Copy link
Contributor Author

costela commented May 31, 2024

Friendly reminder 😇

Now that the iter package is apparently being released as GA in 1.23 (at least it's in master and mentioned in the current draft release notes), it may be a good idea to reconsider this approach?

@costela
Copy link
Contributor Author

costela commented Aug 14, 2024

And now 1.23 is out with rangefunc support 🎉

Any feedback here? 😇

@jakubbortlik
Copy link

And now 1.23 is out with rangefunc support 🎉

Any feedback here? 😇

Yeah, it would indeed be very nice if a pagination helper was implemented. Either this one or #1741.

It would also make it possible to close one more MRs - #1498 :-)

@svanharmelen
Copy link
Member

If we can clean this PR up a little (mainly the comments I think) I'm good with merging this one...

@costela
Copy link
Contributor Author

costela commented Sep 25, 2024

@svanharmelen I see only this open thread, which has been addressed. Am I missing something? 🤔

Also: should we keep the non-rangefunc variant, or should we now go "all-in" and just provide the a rangefunc variant of AllPages? I personally would go this way, to not add unnecessary ambiguity to the API.

@svanharmelen
Copy link
Member

svanharmelen commented Sep 26, 2024

I was looking at these:
image

But also noticed the comments don't follow the contributing guidelines (keep comment lines below 80 chars).

@svanharmelen
Copy link
Member

I must admit that maybe I was a bit too quick yesterday, as I now realize that choosing this solution would actually make this package require at least Go 1.23.x which might cause issues for some of the people using this package.

@costela
Copy link
Contributor Author

costela commented Sep 26, 2024

choosing this solution would actually make this package require at least Go 1.23.x

Well, not necessarily? I'd keep the //go:build tags so as to only make the functionality available for newer versions. That means no bump in go.mod is needed and people that must remain on older versions can still use the rest of the lib without issues.
I'd still avoid providing both solutions though. I think the value of a having a single solution outweights the value of supporting older go versions.

As for the comments: gotcha! I'll clean them up 👍

@costela costela force-pushed the add-all-pages-pagination-helper branch from f2608d2 to 944529b Compare September 28, 2024 09:25
@costela
Copy link
Contributor Author

costela commented Sep 28, 2024

@svanharmelen cleaned the comments and removed the non-rangefunc variant. Now this PR focus only on the iterator support. Also added some basic tests. PTAL 🙏

Reminder: as mentioned above, the two variants introduced in this PR cover only about 60% of the list-like methods in the API. There's probably some room to refactor the other methods and make them all a bit more similar, so that in the future more lists can be paginated with the same iterator.

@costela
Copy link
Contributor Author

costela commented Oct 14, 2024

@svanharmelen friendly ping 😇
anything I missed from the reviews above?

@svanharmelen
Copy link
Member

Thanks for the ping and sorry for taking so long with this one... This just falls outside of "standard" fixes and enhancements and I want to take some time to play with it myself. Yet I'm not using GitLab myself anymore (for a long long time by now 🙈) so sitting myself down to actually do that in between "work" turns out to be something that doesn't happen by itself 😏

I will try to make some time for it in the coming week(end)-ish 😉 And again sorry for all the delays and thank you for your patience!

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.

5 participants