Skip to content

Fix ListPullRequestsWithCommit option type#2822

Merged
gmlewis merged 2 commits intogoogle:masterfrom
RickleAndMortimer:list_pr_options_bugfix
Jul 3, 2023
Merged

Fix ListPullRequestsWithCommit option type#2822
gmlewis merged 2 commits intogoogle:masterfrom
RickleAndMortimer:list_pr_options_bugfix

Conversation

@RickleAndMortimer
Copy link
Copy Markdown
Contributor

@RickleAndMortimer RickleAndMortimer commented Jun 28, 2023 •
edited
Loading

ListPullRequestsWithCommit uses PullRequestListOptions as one of it's parameters. This is incorrect because in GitHub's documentation, the endpoint for this function, repos/%v/%v/commits/%v/pulls, does not share the same parameters as the ones defined in ListPullRequestsWithCommit

go-github/github/pulls.go

Lines 119 to 141 in 96726d8

type PullRequestListOptions struct {
// State filters pull requests based on their state. Possible values are:
// open, closed, all. Default is "open".
State string `url:"state,omitempty"`
// Head filters pull requests by head user and branch name in the format of:
// "user:ref-name".
Head string `url:"head,omitempty"`
// Base filters pull requests by base branch name.
Base string `url:"base,omitempty"`
// Sort specifies how to sort pull requests. Possible values are: created,
// updated, popularity, long-running. Default is "created".
Sort string `url:"sort,omitempty"`
// Direction in which to sort pull requests. Possible values are: asc, desc.
// If Sort is "created" or not specified, Default is "desc", otherwise Default
// is "asc"
Direction string `url:"direction,omitempty"`
ListOptions
}

image

This PR simply sets the correct options, ListOptions, for the ListPullRequestWithCommit.

see #2815 for more details

@RickleAndMortimer @vgnshiyer
replaced PullRequestListOptions param with ListOptions
90083cd 
Co-Authored-By: vgnshiyer <vgnshiyer@asu.edu>
@gmlewis gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Jun 28, 2023
@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Jun 28, 2023

Thank you, @RickleAndMortimer !
Please run gofmt on the files you edited and push (not force push) the changes to this PR.
See CONTRIBUTING.md for more details.

@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Jun 28, 2023

Fixes: #2815.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 28, 2023

Codecov Report

Merging #2822 (2dd9f5c) into master (96726d8) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2822   +/-   ##
=======================================
  Coverage   98.06%   98.06%           
=======================================
  Files         136      136           
  Lines       12279    12279           
=======================================
  Hits        12041    12041           
  Misses        162      162           
  Partials       76       76
Impacted Files Coverage Δ
github/pulls.go 96.77% <100.00%> (ø)

... and 1 file with indirect coverage changes

gmlewis
gmlewis approved these changes Jun 28, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @RickleAndMortimer !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

valbeat
valbeat approved these changes Jul 3, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@valbeat valbeat left a comment

Choose a reason for hiding this comment

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

LGTM!

@gmlewis gmlewis changed the title ListPullRequestsWithCommit has incorrect option type Fix ListPullRequestsWithCommit option type Jul 3, 2023
@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jul 3, 2023
@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Jul 3, 2023

Thank you, @valbeat !
Merging.

@gmlewis gmlewis merged commit 9f7124c into google:master Jul 3, 2023
@RickleAndMortimer RickleAndMortimer deleted the list_pr_options_bugfix branch July 10, 2023 00:35
@gmlewis gmlewis mentioned this pull request Aug 16, 2023
Merged
@asvoboda asvoboda mentioned this pull request Sep 11, 2023
Merged
suzuki-shunsuke added a commit to suzuki-shunsuke/tfcmt that referenced this pull request Oct 7, 2023
@suzuki-shunsuke
fix: address API breaking changes
66fce6d 
- google/go-github#2822
suzuki-shunsuke added a commit to suzuki-shunsuke/tfcmt that referenced this pull request Oct 7, 2023
@renovate @suzuki-shunsuke
fix(deps): update module github.com/google/go-github/v53 to v55 (#890)
3e7f2ec 
* fix(deps): update module github.com/google/go-github/v53 to v55

* fix: address API breaking changes

- google/go-github#2822

* fix: stop using deprecated API github.NewEnterpriseClient

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Shunsuke Suzuki <suzuki.shunsuke.1989@gmail.com>
evanelias added a commit to evanelias/go-github that referenced this pull request Feb 5, 2025
@evanelias
docs: clarify ListPullRequestsWithCommit usage
bd4212e 
The docs for ListPullRequestsWithCommit mention use of PullRequestListOptions,
but this method no longer accepts PullRequestListOptions per google#2815 and google#2822.

This commit updates the method doc comment to remove the outdated reference
to PullRequestListOptions, and adds some clarifications based on the GitHub
docs for the relevant API endpoint.
@evanelias evanelias mentioned this pull request Feb 5, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlewis gmlewis gmlewis approved these changes

+1 more reviewer

@valbeat valbeat valbeat approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@RickleAndMortimer @gmlewis @valbeat