Having ListOptions and ListCursorOptions in AlertListOptions creates ambiguity

[himazawa]

Since both ListOptions and ListCursorOptions implements Page and PerPage there is an ambiguity for both fields.

go-github/github/code-scanning.go

Line 144 in 90bf432

ListCursorOptions

go-github/github/code-scanning.go

Line 148 in 90bf432

ListOptions

My suggestion would be to add the explicit reference to the struct.

[gmlewis]

I agree that this is ambiguous, confusing, and unnecessary.

Looking at the two endpoints that use these options, both of them take page and per_page, so I think ListCursorOptions should be completely removed and only ListOptions should remain.

@himazawa - would you like to make a PR, or shall I open this up to other contributors to this repo (or maybe just do this one myself when I get a chance)?

[himazawa]

Yes I can look into it but looks like the issue is bigger since looks like resp.NextPage stays to 0 even if there are pages.

Check this repro:

func GetSecAlerts(org string){
client := github.NewClient(nil).WithAuthToken(os.Getenv("GH_PAT"))
	opts := &github.AlertListOptions{
		ListOptions: github.ListOptions{
			PerPage: 100,
		},
	}
	ctx := context.Background()
	for {
		results, resp, err := client.CodeScanning.ListAlertsForOrg(ctx, org, opts)
		fmt.Println(resp.LastPage, resp.NextPage) //nextPage stays at 0 even if there are more pages
		if err != nil {
			log.Fatalln(err)
		}
		if resp.NextPage == 0 {
			break
		}
		opts.ListOptions.Page = resp.NextPage
	}

}

[gmlewis]

Hmmm... Maybe I'll have to dig through the history of this... It might be a known issue where both are needed.

At the very least, we should add a comment that better explains the usage and nuances.

[gmlewis]

Ah yes, I see that a comment was indeed added here:

020d9ae

and this was added as a result of #2512.

So I think this is the correct solution, but maybe the comments could be improved.
@himazawa - do you want to write a PR to improve the comments?

[himazawa]

Yea @gmlewis,
I was looking at the same issue you linked.

There is no way to paginate that endpoint using ListOptions.

The doc doesn't seem so clear about it tho.
Something like that should work:

func GetOrgSecurityAlert(org string) {
	client := github.NewClient(nil).WithAuthToken(os.Getenv("GH_PAT"))
	opts := &github.AlertListOptions{
		ListCursorOptions: github.ListCursorOptions{
			PerPage: 100,
		},
	}
	ctx := context.Background()
	for {
		results, resp, err := client.CodeScanning.ListAlertsForOrg(ctx, org, opts)
		if err != nil {
			log.Fatalln(err)
		}
		fmt.Println(resp.Cursor, resp.Before, resp.After)
		if resp.Before == resp.After {
			break
		}

		opts.ListCursorOptions.After = resp.After
	}

}

I'm not sure if checking resp.Before == resp.After is the correct way to break out of the pagination and can't find a reference for that.

What we could do is:

• Add an example on how to paginate using ListCursorOptions in the README
• Explicitly split ListCursorOptions and ListOptions when inside the same struct so the ambiguity is solved
• Add comments about it

What do you think about that?

[gmlewis]

I think that sounds like a good plan if you can make sure in unit tests that past examples from the discussions in #2511 and #2446 are all taken into account and that we don't further break things. (It's OK if we make a breaking API change to support this, but I'm talking about not further breaking existing functionality, to be clear.)

[himazawa]

I don't think it will break anything, indeed the ambiguity is only trying to call opts.Page and opts.PerPage so just putting them explicitly shouldn't break anything, let's see