Skip to content

Conversation

@merchantmoh-debug
Copy link
Contributor

@merchantmoh-debug merchantmoh-debug commented Jan 14, 2026

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:

opt := &github.RepositoryListByUserOptions{ListOptions: github.ListOptions{PerPage: 10}}
for {
	repos, resp, err := client.Repositories.ListByUser(ctx, "user", opt)
	// handle err
	for _, repo := range repos { ... }
	if resp.NextPage == 0 { break }
	opt.Page = resp.NextPage
}

The Solution: With this PR, users can simply write:

for repo, err := range client.Repositories.ListByUserIter(ctx, "user", opt) {
	if err != nil { log.Fatal(err) }
	fmt.Println(repo.GetName())
}

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.

google-labs-jules bot and others added 3 commits January 14, 2026 08:54
- Create tools/gen-iterators to generate type-safe iterators for all List methods.
- Generate github/iterators.go containing generic iter.Seq2 wrappers.
- Update tools/metadata to ignore generated iterators (avoiding metadata duplication).
- Add tests verifying pagination logic.
- Create tools/gen-iterators to generate type-safe iterators for all List methods.
- Generate github/iterators.go containing generic iter.Seq2 wrappers.
- Update tools/metadata to ignore generated iterators (avoiding metadata duplication).
- Add table-driven tests verifying pagination and safety (copying opts).
- Add benchmark comparing iterators to manual pagination.
- Add Godoc example.
@merchantmoh-debug
Copy link
Contributor Author

@gmlewis - @alexandear

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
Copy link

codecov bot commented Jan 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.47%. Comparing base (5458fbc) to head (b5bac31).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3916      +/-   ##
==========================================
- Coverage   92.45%   85.47%   -6.99%     
==========================================
  Files         203      204       +1     
  Lines       14954    17565    +2611     
==========================================
+ Hits        13826    15014    +1188     
- Misses        926     1917     +991     
- Partials      202      634     +432     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Jan 15, 2026
@merchantmoh-debug
Copy link
Contributor Author

@alexandear - @gmlewis = Thanks for the review! I've addressed all the feedback:

  1. Generator Location: Confirmed gen-iterators.go is in the github/ package, matching the pattern of gen-accessors.go.
  2. Go Generate: Added //go:generate go run gen-iterators.go to github/github.go so it runs automatically with other generators.
  3. Test Coverage: The generator now outputs a corresponding iterators_gen_test.go file. This includes a specific test case for every single generated iterator method, ensuring we have 100% coverage of the new functionality.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Stringify optimization changes here are redundant with those added in #3914 .
Would it make sense to keep them in #3914 only and limit this one to iterator support?

Copy link
Contributor Author

@merchantmoh-debug merchantmoh-debug Jan 15, 2026

Choose a reason for hiding this comment

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

It's just Jules being annoying. Got stuck adding them to the wrong branch despite explicit instructions to keep it separate. - One branch for the stringify performance upgrade & one for this. Probably should start new sessions for each branch instead of sticking to one jules task for both.

Anyway I reverted them & I'm applying them to the correct branch now. Sorry for the mix up. Google Labs got some amazing products but they're still working out the kinks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@merchantmoh-debug - it appears that those changes are still leaking through to this PR.

Please take a look at this PR in the GitHub user interface and you will see what we are seeing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmlewis - Yeah; It's a mess. Still learning the Github UI myself. That's why I let Jules handle it.

Boy oh boy did he give me a lot of work to do. That's AI for you. Helpful 90% of the time then it crashes your entire project lol! I'll fix it as soon as I wake up. Need to sleep - I'm on "owl" mode right now.

Copy link
Collaborator

@gmlewis gmlewis Jan 15, 2026

Choose a reason for hiding this comment

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

Yes, just remember that AI usage can be thought of as another tool in the code developer's toolbox and still needs to be used carefully... and the results must be reviewed by the developer. I find the experience very similar to having a coworker that you delegate the job to... sometimes there are communication challenges or the other coworker simply doesn't have the same understanding or mental picture that you assume they do, and so the results don't always turn out like you expected. Thus, we as developers must still review everything.

Copy link
Collaborator

@gmlewis gmlewis left a 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.

@@ -0,0 +1,29 @@
// Copyright 2025 The go-github AUTHORS. All rights reserved.
Copy link
Collaborator

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 ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes >.<; good catch.

@@ -0,0 +1,455 @@
// Copyright 2025 The go-github AUTHORS. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto for this file and all NEW files. If a file already existed, please do not change its copyright year.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha.

// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
// Code generated by gen-iterators; DO NOT EDIT.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make this the first line. See suggestion above.

// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
// Code generated by gen-iterators; DO NOT EDIT.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make this the first line. See suggestion above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@merchantmoh-debug - it appears that those changes are still leaking through to this PR.

Please take a look at this PR in the GitHub user interface and you will see what we are seeing.

# golangci-lint -v custom generates the following local file:
custom-gcl
custom-gcl.exe
gen-iterators
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the addition of //go:generate, this executable should never have to be built, and therefore this line is unnecessary. Please remove.

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 15, 2026

You can completely ignore this suggestion from the email, as it will be taken care of by one of the other suggestions:

-	ctx := context.Background()
-	_ = ctx // avoid unused
-
-	// Call iterator with zero values
-	iter := client.Users.ListUserSocialAccountsIter(context.Background(), "", nil)
+	ctx := context.Background()
+
+	// Call iterator with zero values
+	iter := client.Users.ListUserSocialAccountsIter(ctx, "", nil)

@alexandear

This comment was marked as outdated.

@alexandear
Copy link
Contributor

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:

  • no need to generate iterators and their tests
  • less code to maintain

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 15, 2026

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:

  • no need to generate iterators and their tests
  • less code to maintain

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NeedsReview PR is awaiting a review before merging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants