-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: Add native Go 1.23 Iterator support #3916
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 native Go 1.23 Iterator support #3916
Conversation
|
Review when you can guys. Hoping this feature closes the gap between GO and AWS/Azure. Happy to help :) - Let me know of any changes, hiccups or additions you want. Let's keep the momentum up. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3916 +/- ##
==========================================
- Coverage 92.45% 85.51% -6.95%
==========================================
Files 203 204 +1
Lines 14980 17554 +2574
==========================================
+ Hits 13850 15011 +1161
- Misses 927 1913 +986
- Partials 203 630 +427 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@alexandear - @gmlewis = Thanks for the review! I've addressed all the feedback:
@Not-Dhananjay-Mishra - Thanks for catching that! It looks like an accidental regression occurred where DependencySBOMCategory (and its RateLimits field) was dropped, likely due to a merge/rebase issue with my local branch. I have restored the DependencySBOMCategory constant, the GetRateLimitCategory case, and the DependencySBOM field in the RateLimits struct. Tests now pass. I will update the PR shortly. |
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, @merchantmoh-debug - this is looking very promising.
github/example_iterators_test.go
Outdated
| @@ -0,0 +1,29 @@ | |||
| // Copyright 2025 The go-github AUTHORS. All rights reserved. | |||
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.
All new source files created in 2026 should have // Copyright 2026 ....
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.
Yes >.<; good catch.
|
You can completely ignore this suggestion from the email, as it will be taken care of by one of the other suggestions: |
This comment was marked as outdated.
This comment was marked as outdated.
|
I know it's not ideal to discuss this inside a PR, but we don’t have an issue for this feature, so this is the only place to do it. Do we really need to generate iterators at all? Maybe we could provide helper functions similar to what we have in the GitLab API client: Scan, Scan2, ScanAndCollect? The benefits would be:
|
That is an intriguing suggestion, @alexandear! There have been many attempts to discuss adding automated pagination to this repo in the past, including:
If you are willing, it might be nice to put together a PR that demonstrates this alternative mechanism and we can compare the two approaches. Would you be willing to work on that, @alexandear? |
Working on it in #3925 |
|
@gmlewis @alexandear - Well; that was a long arse sleep? hahahah. Sorry. My ADHD took me elsewhere and my Autistic hyperfocus kept me there for longer then I wanted. Mix up reverted. Let's continue shall we? |
|
@gmlewis Updates pushed to address all review feedback and build issues: Build Fix: Patched the generator to explicitly skip ListMatchingRefs, which was causing the undefined: ReferenceListOptions compilation error. |
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.
@merchantmoh-debug - this PR has the same issues as your other one.
Please take a look at the GitHub user interface to see all the files included in your PR.
|
@gmlewis - Gotcha |
bdb5a29 to
c73b01c
Compare
|
@gmlewis - So I reset. Took only the changed files and added them exclusively. Good? Github interface sucks arse. (Rather I just suck arse and I'm blaming github lmao) |
github/gen-iterators.go
Outdated
| ctx := context.Background() | ||
| _ = ctx // avoid unused | ||
|
|
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.
These 3 lines can still be deleted.
c73b01c to
bdb5a29
Compare
|
@gmlewis Done. |
Co-authored-by: Oleksandr Redko <[email protected]>
|
@alexandear @gmlewis - Working on the Otel right now I'll shift back to this in a little while. - suggested commits committed Alex. |
Co-authored-by: Oleksandr Redko <[email protected]>
d4270ad to
e10b9f5
Compare
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, @merchantmoh-debug!
LGTM.
I propose that we move forward with this integer-pagination-only mechanism first before tackling cusor-based-pagination in a separate PR.
I would like to hear from our regular reviewers before merging:
alexandear
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.
Should we also update https://github.com/google/go-github#iterators-experimental?
I'm agree that we can limit this PR for the pagination-only iterator.
|
|
||
| //go:build ignore | ||
|
|
||
| // gen-iterators generates iterator methods for List methods. |
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.
We should describe more. Especially let's comment about limitations and assumptions.
|
This PR contains a lot of discussions that would be better placed in an issue. @merchantmoh-debug, for the future, if you want to add a new major feature, please create an issue first. |
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.
This file should be named as the complement to iterators.go: iterators_test.go (not iterators_gen_test.go).
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.
How about github-iterators.go and github-iterators_test.go ?
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.
@gmlewis agree
|
@gmlewis could you address all the listed comments instead? To speed up the process. |
Co-authored-by: Oleksandr Redko <[email protected]>
Unfortunately, I have to step away from the computer for a few hours, but then if they are not updated, I will attempt to work on these, although I can no longer rename files. I also want to dig deep and understand what happens during rate limiting during actual iterator usage when I come back. |
|
OK, I'm back and I believe that rate limiting can all be handled by the transport. Additionally, I'm thinking of possibly merging this PR and then I would create a follow-on PR to address the remaining issues. |
|
@gmlewis - I agree. That's likely the best way to do it. If @alexandear & @Not-Dhananjay-Mishra - I'm onboard. |
Looks good. Let's merge. |
|
@alexandear - I'm kinda a noob to Codecov --- does it sometimes glitch? Just wondering if satisfying it until it has a green check mark matters -- never dealt with it before |
Solid, I agree 👍 |
|
@gmlewis -And we have lift off! Amazing work guys! |
Yes, Codecov seems to be a very delicate thing and I've had quite a history with it. |
|
Excellent. Thank you, everyone. Merging now and I will post a follow-up PR soon. |
|
@gmlewis - Thanks - that's what I read online as well! 2/3rds done. Maybe Google will offer me a job 😂 😂 |
This PR introduces native Go 1.23 iterator support to the entire library via a new code generator.
The Problem
Pagination in go-github currently requires verbose boilerplate:
The Solution: With this PR, users can simply write:
Implementation Details
github/gen-iterators.go: A new AST-based tool (similar to gen-accessors) that scans all List* methods, identifies their pagination patterns (Page/Cursor), and generates a corresponding *Iter method returning iter.Seq2.
It handles methods using standard ListOptions embedding and explicit Page fields.
It copies the opts struct to avoid mutating the caller's options during iteration.
github/iterators.go: The generated file containing hundreds of type-safe iterators.
Metadata Handling: Updated tools/metadata to exclude the generated iterators from API operation mapping validation, as they wrap existing operations.
Testing & Benchmarks
github/iterators_benchmark_test.go confirms that the iterator abstraction introduces negligible overhead compared to manual looping.
github/iterators_test.go verifies single-page, multi-page, and error handling scenarios.
Tests explicitly verify that the user's opts struct is NOT mutated by the iterator.
Documentation
Added github/example_iterators_test.go to provide a clear example in the Godoc.
This modernizes the library to leverage the latest Go features, significantly improving Developer Experience (DX) by eliminating error-prone pagination boilerplate.