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

Improve the Octo pr list command #546

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alexkalderimis
Copy link

@alexkalderimis alexkalderimis commented May 23, 2024

Describe what this PR does / why we need it

This PR includes improvements to the pr list command. Specifically:

  • it changes the fetch implementation to one based on REST rather than GraphQL, since GraphQL lacks several important features, such as the ability to filter on authors or assignees or labels.
  • it abandons the attempt to fetch all PRs via pagination, since repos with hundreds or even thousands of PRs will cause this to be inefficient or unusable
  • it changes the display in telescope of PRs to show authors and branches, and allows the user to filter based on these values. This is very important in organizations where practices such as including ticket IDs in branch names are followed.

Does this pull request fix one issue?

Fixes #551

Describe how you did it

The main changes are in pickers/telescope/entry_maker.lua and pickers/telescope/provider.lua. These files define both the mechanism for retrieving pull requests and how they are passed to the telescope picker.

A new function gen_from_pull_request is added in entry_maker.lua to handle the display of the pull request data, including the addition of the new author and branch columns. The ordinal for pull requests now includes the author and branch information, allowing this to be used in filtering (displaying this is not enough).

The retrieval is changed in telescope/provider.lua (specifically in the pull_requests function). Here we call gh pr list rather than using the exisiting GraphQL implementation in order to provide access to the additional features that allows.

The retrieved records are modified to conform to the GraphQL result schema, meaning that they can be used in existing functions if necessary.

Describe how to verify it

The following actions will be useful in verification:

  • run Octo pr list and then use telescope to filter by author name
  • run Octo pr list author=@me and see that only your PRs are shown

Special notes for reviews

Prior to this change, we were using the GraphQL resources to retrieve
pull requests. This is limited, since there are a number of very useful
queries that are not available in GraphQL, including filtering PRs by
author or assignee.

This change replaces the GraphQL implementation with one based on
`gh pr list`, which uses the REST API, and is more functional.

This allows filtering by author, assignee, branches (both head and
base), state and labels, as well as arbitrary search strings.

To handle large repos gracefully, we abandon the attempt to retrieve all
pull requests by paginating, since this is potentially unbounded.
Instead we fetch at most a configurable limit of pull requests, and
expect the user to use filtering on the list to improve relevance.
@@ -127,6 +127,7 @@ function M.get_default_values()
field = "CREATED_AT",
direction = "DESC",
},
limit = 50,
Copy link
Owner

Choose a reason for hiding this comment

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

Please add to the docs and validate is a number

utils.info "Fetching pull requests (this may take a while) ..."
local args = {
"pr", "list",
"--limit", tostring(opts.limit),
Copy link
Owner

Choose a reason for hiding this comment

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

Should we add --paginate in case limit is bigger than 100?

pull.author.username = pull.author.login
pull.repository = { nameWithOwner = pull.headRepositoryOwner.login .. "/" .. pull.headRepository.name }

authors[pull.author.id] = (authors[pull.author.id] or 0) + 1
Copy link
Owner

Choose a reason for hiding this comment

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

Im testing Octo pr list for the neovim/neovim repo and getting the following error in this line:

Error executing vim.schedule lua callback: ...tester/octo.nvim/lua/octo/pickers/telescope/provider.lua:319: table index is nil
stack traceback:
        ...tester/octo.nvim/lua/octo/pickers/telescope/provider.lua:319: in function 'cb'
        .../src/github.com/pwntester/octo.nvim/lua/octo/gh/init.lua:161: in function ''
        vim/_editor.lua: in function <vim/_editor.lua:0>

Seems like id may be nil and needs to be handled

@pwntester
Copy link
Owner

Thanks for the PR! added a few comments

pull.author.username = pull.author.login
pull.repository = { nameWithOwner = pull.headRepositoryOwner.login .. "/" .. pull.headRepository.name }

authors[pull.author.id] = (authors[pull.author.id] or 0) + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
authors[pull.author.id] = (authors[pull.author.id] or 0) + 1
authorid = pull.author.id or 0
authors[pull.author.id] = (authors[pull.author.id] or 0) + 1

@legobeat
Copy link
Contributor

legobeat commented Jul 9, 2024

Thank you! FWIW this works fine for me with the suggested patch.

@pwntester
Copy link
Owner

Hey @alexkalderimis can you take a look at the review comments and patch provided by @legobeat ?

@macovsky
Copy link
Contributor

@alexkalderimis hi there, are you going to move this forward? if not, I can try to address the comments.

@wd60622
Copy link
Collaborator

wd60622 commented Oct 13, 2024

If you need any support, feel free to tag me. I will be happy to help where I can!

@legobeat
Copy link
Contributor

Resubmitted this with feedback addressed in #635

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.

Octo pr list: Not possible to filter by branch or author in PR picker
5 participants