Missing iterators

[stevehipwell]

It looks like the current auto generated iterators are only being created for the list commands, but the GitHub API uses pagination for other endpoints such as get a commit (client.Repositories.GetCommit()).

[gmlewis]

Yes, that was a conscious design decision:

		if !fd.Name.IsExported() || !strings.HasPrefix(fd.Name.Name, "List") {
			continue
		}

The plan was to make this feature extremely consistent across the entire repo.
So if we wanted to make iteration available for other methods, we would make a breaking change and force it to conform to this standard.

[stevehipwell]

Yes, that was a conscious design decision

Can you remember the reasons? Wouldn't creating an iterator function for any exported function that has one of the list options as an argument also be consistent?

[gmlewis]

Yes, it was just so that users can easily look for List*Iter methods, and if the method starts with List* it might have an iterator.

Can you remember the reasons? Wouldn't creating an iterator function for any exported function that has one of the list options as an argument also be consistent?

Yes, that is absolutely also a viable alternative design decision.

We had others weigh in on this decision previously while we were originally creating the feature...

cc: @alexandear - @zyfy29 - @Not-Dhananjay-Mishra - @munlicode - Does anyone have any opinions about changing our design decisions made for the current set of iterators to accomodate this alternate design?

I'm not married to the current approach but I do believe we discussed this previously.

[munlicode]

I think that creating a function, as @stevehipwell suggested, would be great, since it makes the library more resilient in case function, for some reason, needs to attain its name.

We could potentially add verbose command that would show what functions do not have List*Iter or List* naming style, so that it is just clear what functions are not consistent in naming, though it is more of an enchantment.

[Not-Dhananjay-Mishra]

I have a question even if we allow iterator generation for methods without a List* prefix (like GetCommit), it returns a *RepositoryCommit, which is not a slice. Instead, it contains two slice fields Parents []*Commit and Files []*CommitFile. How will we handle this case?With the current implementation, only structs with a single slice field are supported. I guess there will be more cases like this.

[stevehipwell]

@Not-Dhananjay-Mishra when you page the commit API you get the same object back but with the next set of files. Wouldn't that be the implementation here?

[Not-Dhananjay-Mishra]

@Not-Dhananjay-Mishra when you page the commit API you get the same object back but with the next set of files. Wouldn't that be the implementation here?

I agree with that, we should have iterator for that.
Just GetCommitIter() might be confusing name for a iterator that iterate over files of a commit.
Could we use a different name for the iterator in cases like this?

[alexandear]

Other examples are (found with ripgrep: rg -p "Get.*opts \*ListOptions"):

• RepositoriesService.GetRulesForBranch

  go-github/github/repos_rules.go

  Line 18 in a12b8d7

  func (s *RepositoriesService) GetRulesForBranch(ctx context.Context, owner, repo, branch string, opts *ListOptions) (*BranchRules, *Response, error) {

• RepositoriesService.GetCombinedStatus

  go-github/github/repos_statuses.go

  Line 119 in a12b8d7

  func (s *RepositoriesService) GetCombinedStatus(ctx context.Context, owner, repo, ref string, opts *ListOptions) (*CombinedStatus, *Response, error) {

• EnterpriseService.GetConsumedLicenses

  go-github/github/enterprise_licenses.go

  Line 97 in a12b8d7

  func (s *EnterpriseService) GetConsumedLicenses(ctx context.Context, enterprise string, opts *ListOptions) (*EnterpriseConsumedLicenses, *Response, error) {

• OrganizationsService.GetAllRepositoryRulesets

  go-github/github/orgs_rules.go

  Line 18 in a12b8d7

  func (s *OrganizationsService) GetAllRepositoryRulesets(ctx context.Context, org string, opts *ListOptions) ([]*RepositoryRuleset, *Response, error) {

Given that there are a limited number of such cases, we can handle each separately.

OrganizationsService.GetAllRepositoryRulesets returns a slice and can be renamed to OrganizationsService.ListAllRepositoryRulesets. Of course, it's a breaking change.

For others, we can leave them as is or write generator functions manually.
I would not broaden generation to "any exported method with list options" as a blanket rule. That would weaken the current discoverability story and it also doesn't handle ambiguous wrapper responses well.

[stevehipwell]

So what is the general consensus here for moving forwards then?

[gmlewis]

So what is the general consensus here for moving forwards then?

I think that @alexandear has a good suggestion above and I really do indeed like our current discoverability story where we make iterators out of methods that start with List and are iterable. I think this is easy to remember and easy to understand.

I also have no problems with making breaking API changes (obviously since I've made 84 of them now 😂 ) so it would be totally fine by me to rename these methods and thereby auto-generate iterators for them.

I'm also fine if we use this issue to drive the development and ideally make one PR for each breaking change... that way if we horribly break something other than just the API, we can easily revert.

Before anyone starts on any method, please make a note in this issue so that others don't duplicate the work. Thank you!

[Not-Dhananjay-Mishra]

For others, we can leave them as is or write generator functions manually.

Can we make these map in gen-iterators.go for others

// customNames provides custom names for iterator methods where the default methodName + "Iter" would be confusing.
var customNames = map[string]string{
	"RepositoriesService.GetCommit":         "ListCommitFiles",
	"RepositoriesService.CompareCommits":    "ListCommitComparisonFiles",
	"RepositoriesService.GetCombinedStatus": "ListCombinedStatuses",
}

// sliceToBeUsedForIteration identifies methods where the wrapper struct contains multiple []*T fields,
// and specifies which field should be used for iteration.
var sliceToBeUsedForIteration = map[string]string{
	"RepositoriesService.GetCommit":      "Files",
	"RepositoriesService.CompareCommits": "Files",
}

with customNames we can assign custom names specifically for the iterator while keeping the original method name unchanged also with this we can give more understandable names for the iterator.

and some method like GetCommit and CompareCommits contains multiple []*T fields with sliceToBeUsedForIteration we can specify which field should be used for iteration