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.