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

ListPullRequestsWithCommit has incorrect option type #2815

Closed
bennesp opened this issue Jun 20, 2023 · 9 comments
Closed

ListPullRequestsWithCommit has incorrect option type #2815

bennesp opened this issue Jun 20, 2023 · 9 comments
Assignees
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). bug good first issue

Comments

@bennesp
Copy link

bennesp commented Jun 20, 2023

I am currently using PullRequests.ListPullRequestsWithCommit to get a PR starting from a commit hash.

This function accepts a parameter "options" of type PullRequestListOptions. PullRequestListOptions has the State field along with many other fields, but State does not seem to do anything at all to the results of the function.

Example using commit sha cc8044e (PR #2814, a merged PR from this repository):

func main() {
	client := github.NewClient(nil)

	sha := "cc8044e88926ef5b8ddb447c9a9fc5b2c8d6df4d"
	options := &github.PullRequestListOptions{
		State: "open",
	}
	pull_requests, _, err := client.PullRequests.ListPullRequestsWithCommit(context.TODO(), "google", "go-github", sha, options)
	if err != nil {
		panic(err)
	}
	if len(pull_requests) == 0 {
		fmt.Println("no pull requests found")
		return
	}

	fmt.Printf("First PR: #%d\n", *pull_requests[0].Number)
}

This will print First PR: #2814, which seems to me to be incorrect because it is a merged PR, and thus its state is "closed", not "open".

This seems to be because this function uses the REST API "repos/%v/%v/commits/%v/pulls" under the hood, which, as stated in the documentation, being an API from the Commits API, not from the Pull Request API, it does not have a state parameter.

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 20, 2023

Thank you, @bennesp !
Do you want to create a PR to fix this, or shall I open this issue to other contributors to this repo?

@bennesp
Copy link
Author

bennesp commented Jun 20, 2023

Unfortunately, right now I don't think I am able to follow up on creating a PR for this, so I would rather open the issue to other contributors. If noone will pick this, then probably in the future I might have the chance to create a PR.

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 20, 2023

This would be a great PR for any new contributor to this repo or a new Go developer.
All contributions are greatly appreciated!

Feel free to volunteer for any issue and the issue can be assigned to you so that others don't attempt to duplicate the work.

Please check out our CONTRIBUTING.md guide to get started. (In particular, please remember to go generate ./... and don't use force-push to your PRs.)

Thank you!

@RickleAndMortimer
Copy link
Contributor

Hey all, I'm interested in working on this issue

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 20, 2023

Hey all, I'm interested in working on this issue

Thank you, @RickleAndMortimer !
It's yours.

@vgnshiyer
Copy link

I have a fix for this issue. @RickleAndMortimer Please let me know are you still working on it.

I am happy to collaborate with you in fixing it.

@RickleAndMortimer
Copy link
Contributor

Hey @vgnshiyer! I'd love to chat with you about your process in fixing it. I've been struggling to work with Go for a bit now and I'm a bit busy right now so I'll let you know when we can talk more about it

@vgnshiyer
Copy link

vgnshiyer commented Jun 28, 2023

@RickleAndMortimer Sounds good.

@gmlewis Can you please add me as an assignee to this issue?

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 28, 2023

@vgnshiyer - I can't add you, but I can make you the primary. Thank you for checking with @RickleAndMortimer first. The issue is yours.

EDIT - Whups! TIL that I can assign multiple owners in GitHub. 😁 Done.

@gmlewis gmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Jun 28, 2023
@gmlewis gmlewis closed this as completed in 9f7124c Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). bug good first issue
Projects
None yet
Development

No branches or pull requests

4 participants