Improve the Octo pr list command

[legobeat]

• Based on Improve the Octo pr list command #546 with original work done by @alexkalderimis
• Addressed review feedback by @pwntester
  • Add --paginate to API call
  • Validate and document new configuration field pull_requests.limit
  • Handle nil pull_request author, author.id
• Handle nil pull_request author.username
• Change default pull_requests.limit from 50 to 100

[macovsky]

@legobeat thanks so much for this!

[wd60622]

This doesn't work for me still. I commented out the "--paginate" as it isn't a flag in the command used. I also prefer the previous entry_maker a bit more. Plus with the colors & symbols here: #637

[wd60622]

Also noting that this doesn't impact the Octo pr search command. Is that as intended?

[wd60622]

I think much of the original requirements can be done with the current Octo pr search and being combined with the advanced search functionality: https://docs.github.com/en/search-github/searching-on-github/searching-issues-and-pull-requests

If the goal is to have it in the fuzzy finder and move away from GitHub search then that is another item.

What do people think?

[macovsky]

I think much of the original requirements can be done with the current Octo pr search and being combined with the advanced search functionality: https://docs.github.com/en/search-github/searching-on-github/searching-issues-and-pull-requests

one slight problem with Octo pr search is that one can't merge/checkout PRs right in telescope with picker bindings (these bindings work for me only in Octo pr list)

If the goal is to have it in the fuzzy finder and move away from GitHub search then that is another item.

What do people think?

IMHO it's nice to have branch/author in fuzzy finder

[wd60622]

IMHO it's nice to have branch/author in fuzzy finder

I guess I find the picker a bit cumbersome in the current form. How about adding the formatting of the user and the branch as done in the PR / issue details in order to provide some separation of the row information.

one slight problem with Octo pr search is that one can't merge/checkout PRs right in telescope with picker bindings (these bindings work for me only in Octo pr list)

Adding those bindings could be a good addition and easier solution for this problem.

That would be done here:
https://github.com/wd60622/octo.nvim/blob/88d9fe8e90fa3f5fdeedf4d8d7c1b1abf27f825f/lua/octo/pickers/telescope/provider.lua#L558-L559

in the same manner as:

https://github.com/wd60622/octo.nvim/blob/88d9fe8e90fa3f5fdeedf4d8d7c1b1abf27f825f/lua/octo/pickers/telescope/provider.lua#L341-L344

But ideally, the user would be able to configure these a bit more.

[macovsky]

one slight problem with Octo pr search is that one can't merge/checkout PRs right in telescope with picker bindings (these bindings work for me only in Octo pr list)

Adding those bindings could be a good addition and easier solution for this problem.

That would be done here: https://github.com/wd60622/octo.nvim/blob/88d9fe8e90fa3f5fdeedf4d8d7c1b1abf27f825f/lua/octo/pickers/telescope/provider.lua#L558-L559

in the same manner as:

https://github.com/wd60622/octo.nvim/blob/88d9fe8e90fa3f5fdeedf4d8d7c1b1abf27f825f/lua/octo/pickers/telescope/provider.lua#L341-L344

But ideally, the user would be able to configure these a bit more.

do you mind having a look at #647?

[legobeat]

Apologies for dropping a half-baked version without timely follow-up!

This doesn't work for me still. I commented out the "--paginate" as it isn't a flag in the command used.

Thanks. I have rebased on master now, fixed that oversight and done some manual testing now.

I also prefer the previous entry_maker a bit more. Plus with the colors & symbols here: #637

Nice!

Also noting that this doesn't impact the Octo pr search command. Is that as intended?

There are some tradeoffs with each approach (see description on #546 for some motivations). One one hand I'm not sure too what extent it makes sense to have separate search/list commands with differing behavior and implementation but on the other it is convenient to use it as differentiator between calling graphql or not, as either can be desirable and it would be a chore to have to configure desired default for each repo... So maybe a single unified implementation which can be configured, but still two differing commands with differing defaults is a better approach...?

It looks like there are some snags/bugs (different ones!) in both implementations at this point. Even if existing bugs are fixed I still have situations where this pr list approach is preferred. Open to suggestions on how this PR can be improved in that sense.

How about adding the formatting of the user and the branch as done in the PR / issue details in order to provide some separation of the row information.

I think that sounds like a great idea regardless of all the above! That would solve the motivation of having author name viewable and filterable. Though I guess that would mean that field/column-based sorting would become quite cumbersome?

[pwntester]

Thanks for taking care of this PR @legobeat. It is still on draft state, are you planning on doing any more work on this PR?

[legobeat]

Thanks for taking care of this PR @legobeat. It is still on draft state, are you planning on doing any more work on this PR?

Cheers!

Mostly I put in draft because

1. Waiting for fix(pickers/telescope): handle nil selection in open #662 and fix: handle local filesystem remotes #663 to make manual testing smoother
2. While personally I consider this PR an improvement in its current state, some questions came up here - just want to make sure we're not going too much in another direction than what you intend medium/long-term here!

[pwntester]

Thanks for the update @legobeat We have created a discussion room in Matrix in case you want to join us https://matrix.to/#/#octo.nvim:matrix.org

Also, happy to make you a collaborator if you are up for it