Skip to content

Conversation

@brig
Copy link
Contributor

@brig brig commented Oct 7, 2025

No description provided.

benbroadaway
benbroadaway previously approved these changes Oct 7, 2025
ibodrov
ibodrov previously approved these changes Oct 7, 2025
Copy link
Collaborator

@ibodrov ibodrov left a comment

Choose a reason for hiding this comment

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

Minor comments, otherwise LGTM


var apiUrl = buildApiUrl(apiInfo.baseUrl(), path);
var requestUrl = appendParamsToUrl(apiUrl, effectiveParams);
while (requestUrl != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When requestUrl is null? Should it instead be something like

while (true) {
  var keepGoing = handler.onPage(...)
  if (!keepGoing) {
    break;
  }
  ...etc
}

Copy link
Contributor Author

@brig brig Oct 8, 2025

Choose a reason for hiding this comment

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

requestUrl = parseNextLink(response.headers().firstValue("Link").orElse(null));

We stop either when there are no more pages or when the handler indicates no further processing is needed

@brig brig dismissed stale reviews from ibodrov and benbroadaway via 26f4340 October 8, 2025 00:48
@brig brig added the need docs Documentation required label Oct 9, 2025
@ibodrov ibodrov merged commit 25f48f4 into master Nov 1, 2025
1 check passed
@ibodrov ibodrov deleted the brig/github-list-commits branch November 1, 2025 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

need docs Documentation required

Development

Successfully merging this pull request may close these issues.

4 participants