fix: #4263 show open prs only on default branch#4285
Conversation
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
Looks good! Do you think it will be possible to cover this behavior with a test scoped to findForBranch?
pkg/cmd/pr/shared/finder.go
Outdated
| for _, pr := range prs { | ||
| if pr.HeadLabel() == headBranch && (baseBranch == "" || pr.BaseRefName == baseBranch) { | ||
| return &pr, nil | ||
| if pr.State == "OPEN" || resp.Repository.DefaultBranchRef.Name != headBranch { |
There was a problem hiding this comment.
This condition looks good but could be merged with the if condition that precedes it. Something like:
if pr.HeadLabel() == headBranch && (baseBranch == "" || pr.BaseRefName == baseBranch) && (pr.State == "OPEN" || resp.Repository.DefaultBranchRef.Name != headBranch) {Feel free to split the condition into multiple lines if that looks better
There was a problem hiding this comment.
I am of 2 minds 😅. I can do it however you like.
There was a problem hiding this comment.
Well then just put it on 1-line if condition and then we'll see. As long as it's not nested ifs 👍
I can't think of any way to do that, but I don't have much experience writing tests. If you have any ideas, please help me out. |
|
No worries. We'll try to add a test before this merges ✌️ |
|
I had a look at this fix and from the existing code coverage html report: it seems just this line is not covered after : cli/pkg/cmd/pr/shared/finder.go Line 325 in 43b2785 so a test case that can get to there would effectively run through the new if condition.
I'd be willing to create a patch for |
|
I've created a commit andrewhsu@22d689f with the test. Feel free to cherry-pick or let me know where I should create a PR for this. The commit adds a test case to 2c2
< name: "no argument reads current branch",
---
> name: "current branch with merged pr",
23c23
< "state": "OPEN",
---
> "state": "MERGED",
29c29,32
< ]}
---
> ]},
> "defaultBranchRef":{
> "name": "blueberries"
> }
32,33c35
< wantPR: 13,
< wantRepo: "https://github.com/OWNER/REPO",
---
> wantErr: true,I found https://docs.github.com/en/graphql/overview/explorer helpful because this is my first time diving into GraphQL. Let me know if I've missed the pool. 🏊🏼♂️ |
To ensure only open PRs are shown on default branch when running `gh pr view`.
|
@andrewhsu Much appreciated! @AyushRawal Thank you for your patience 🙇 |
3 participants
Fixes #4263