-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: Add iterator support via Scan and Scan2 #3925
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3925 +/- ##
==========================================
- Coverage 92.45% 92.24% -0.22%
==========================================
Files 203 205 +2
Lines 14980 15067 +87
==========================================
+ Hits 13850 13898 +48
- Misses 927 961 +34
- Partials 203 208 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7205459 to
aa7deb2
Compare
aa7deb2 to
5b330c7
Compare
|
@gmlewis @merchantmoh-debug this is ready for review as a concept, as I was told here. |
gmlewis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working out this PR, @alexandear! I appreciate it.
Here are my initial thoughts:
- this approach is more invasive than I had hoped such that all iterable endpoints will need to be upgraded to support the iterators
- the actual example usage is less intuitive than I had hoped it might be
- looking at the ease-of-use from an end-user's point of view, it seems like the go:generator approach will provide a cleaner and easier-to-understand API
- from a maintenance burden perspective, the two approaches appear comparable and yet I'm guessing that more "Issues" might be filed against this approach helping people to understand how to set up the iteration for their particular use case or just simply because they couldn't wire up their own anonymous function because they couldn't figure out how to copy/paste the return values for the function they are trying to call
Therefore, I'm leaning toward the go:generate approach at this point... but I would like to hear from others as well if they have any opinions.
|
I suppose that a hybrid approach might be to use Scan and Scan2 under the hood but use go:generate to make a really easy-to-use API for users of this repo? Would there be any benefits to this approach over the other go:generate-only approach? |
|
@alexandear @gmlewis I see Scan2 approach support |
@Not-Dhananjay-Mishra You are spot on regarding the current gap. The existing generator template primarily targeted int-based offset pagination (NextPage), but we can trivially extend it to support the cursor variations. |
@gmlewis I strongly believe the pure go:generate approach is superior to both the Scan runtime and a "Hybrid" approach. On usage of "Hybrid" (Generating wrappers around Scan): If we are already running go:generate, we have the opportunity to produce "native" Go loops that call the API directly (Client -> Check Error -> Yield -> Advance). Wrapping Scan would just add a layer of runtime indirection (and potentially reflection) without any DX benefit. Generated code should be simple, readable, and "boring"—it should look exactly like the code a user would write by hand. On Cursor Support: The Response struct actually supports three disjoint cursor models (NextPageToken, Cursor, After). A generator can statically handle this nuance by encoding the precedence logic in the template: go |
Support Go iterator ranging via
ScanandScan2functions.MustIterandScanAndCollectare convenience wrappers. This is an alternative to #3916.Iteration is implemented only for two methods that I was able to verify manually:
IssuesService.ListComments- offset-basedSecurityAdvisoriesService.ListRepositorySecurityAdvisoriesForOrg- cursor (after)-basedOne drawback of this implementation is the requirement to add the optional parameter
reqOpts ...RequestOptionto everyList*endpoint, which is not a backward-compatible change.Adapted from gitlab-org/api/client-go.