Skip to content

Add --auto-merge filtering to gh pr list#7387

Closed
mjpieters wants to merge 2 commits intocli:trunkfrom
mjpieters:auto_merge_gh_pr_list
Closed

Add --auto-merge filtering to gh pr list#7387
mjpieters wants to merge 2 commits intocli:trunkfrom
mjpieters:auto_merge_gh_pr_list

Conversation

@mjpieters
Copy link
Contributor

@mjpieters mjpieters commented Apr 30, 2023

This PR is part of a series for issue #7309, adding auto-merge status support to PR commands

Add the ability to filter the gh pr list PRs on their auto-merge status. There is no server-side support for this, so entries are filtered when retrieving the list of PRs from GraphQL.

Because the list now is (further) filtered on the client side, the total number of PRs after filtering can't be known exactly until you fetch all pages, which would be inefficient and wasteful, so instead the total is marked as an upper bound and the text changes from Showing MATCHED of TOTAL to Showing MATCHED of up to TOTAL unless the last page was reached during local filtering.

$ gh pr list --auto-merge disabled

Showing 30 of 35 pull requests in cli/cli that match your search

...

There are two commits here; the first commit refactors the pr/list/http.go code to consolidate the GraphQL paged-result handling for the two PullRequestList and PullRequestSearch functions into a single fetchPullRequests() function that can handle GraphQL paging for either. This makes it then far easier to add client-side filtering to this single function rather that having to duplicate the effort.

@mjpieters mjpieters requested a review from a team as a code owner April 30, 2023 13:58
@mjpieters mjpieters requested review from vilmibm and removed request for a team April 30, 2023 13:58
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Apr 30, 2023
@mjpieters
Copy link
Contributor Author

Please let me know if the GraphQL paging commit should be split out to a separate PR.

One of these days GitHub is going to implement a decent stacked diffs feature so we can keep interdependent commits together while allowing reviewers to focus on the individual diffs!

@mjpieters mjpieters force-pushed the auto_merge_gh_pr_list branch 3 times, most recently from c460be3 to 3503ef8 Compare May 4, 2023 14:29
mjpieters added 2 commits May 9, 2023 10:40
Consolidate the GraphQL page-fetching loops into one method, with each
variant providing a struct to handle the different response types.
Because the API doesn't support filtering by autoMergeRequest status,
the API responses are filtered locally.
@mjpieters mjpieters force-pushed the auto_merge_gh_pr_list branch from 3503ef8 to 05ce139 Compare May 9, 2023 09:40
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Hmm, I must admit I'm not convinced of the utility of client-side filtering in this case. If a server-side filter does not exist for this, then I would rather opt to not expose this kind of filtering in the list command.

You have already exposed --json autoMergeRequest, so I would vote in favor of consuming that manually.

Others: thoughts?

@samcoe
Copy link
Contributor

samcoe commented May 10, 2023

@mislav I had mostly the same thought as you, which is why I added the discussion label to #7309. I would also vote in favor of consuming the autoMergeRequest field manually.

@mislav
Copy link
Contributor

mislav commented May 11, 2023

Thanks @samcoe.

@mjpieters Closing this because we do not believe that client-side filtering is the way to go. If someone really needs to filter based on auto-merge information, they can consume --json output. Thank you for all your contributions regarding this feature!

@mislav mislav closed this May 11, 2023
@mjpieters mjpieters deleted the auto_merge_gh_pr_list branch May 11, 2023 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external pull request originating outside of the CLI core team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants