Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/comment pagination #53

Draft
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

ataylorme
Copy link

@ataylorme ataylorme commented Feb 2, 2024

What?

Add pagination support for PR comments

Why?

Addresses #41

How?

Use @octokit/plugin-paginate-graphql with plugin support from @actions/core

Notes

I am not sure if this will work as-is. Based on this comment I think the rather large GraphQL query may need to be broken down into multiple, smaller queries for effective pagination.

Probably one GraphQL for each set of PR info needed such as commits, comments, reviews, etc.

@desrosj
Copy link
Contributor

desrosj commented Feb 5, 2024

Thanks for looking at this, @ataylorme!

I think this is the reason for #54.

I've gone and added some test variables to pull the data from the PR experiencing the problem.

I also rebuilt the action after your changes using npm run prepare. You'll need to do that every time you push, otherwise your changes won't be tested by the test workflows.

@desrosj
Copy link
Contributor

desrosj commented Feb 5, 2024

I took another pass at this in #58 looking at the examples in the plugin README file. It's also not there yet.

I did see one thing in the documentation somewhere (struggling to find it again for reference) that made me think we may need to break off a few queries. Nested pagination is not supported. We may need to either:

  • Split up some queries in order to get all the data we need.
  • Check the number of nodes returned for each object type. When the response indicates there's more data, perform a second paginated query starting after: 100 for the rest.

@ataylorme
Copy link
Author

ataylorme commented Feb 5, 2024

@desrosj to truly sort this out, and have confidence in a solution, I would:

  1. Rewrite the plugin in TypeScript
  1. Break up the logic in getContributorData to smaller methods with single responsibility and use getContributorData to call those (e.g. getPullRequestComments, get closingIssuesReferences, etc.). Some advantages are:
  • Reduced cognitive complexity
  • More approachable unit tests
  1. Switch to using the REST API with pagination
  • @ octokit/types.ts combined with 1 and 2 will give strict typing that cannot be achieved with GraphQL

While a single, large graphQL query may be more efficient in reducing the number of API requests the changes above would make the code more maintainable and testable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants