Fix detecting PR status when passing branch as arg#3351
Conversation
mislav
left a comment
There was a problem hiding this comment.
Great catch!
If you don't mind, I'd like to take an approach that avoids the ambiguity of "closed".
api/queries_pr.go
Outdated
| number | ||
| title | ||
| state | ||
| closed |
There was a problem hiding this comment.
The fix looks good, but since the concept of "closed" is ambiguous for pull requests, and we're already fetching the state field, I would say let's go a few extra steps and rely just on the state field and avoid fetching the closed property altogether.
The two non-open states of Pull Requests are "closed" and "merged". Unless explicitly clarified, it can be ambiguous whether "closed" includes "merged" or not. For instance, pr.State == "CLOSED" does not include the "merged" state.
To avoid this ambiguity in our Go code, let's quit querying the closed field and change all pr.Closed checks (3 that I count right now) to actually reflect what they are checking for: the not-open state !pr.IsOpen(). That would involve adding an extra IsOpen() method to PullRequest, but I think it will be worth it. The benefit is that we can fetch one fewer field in GraphQL. What do you think?
There was a problem hiding this comment.
Definitely agree that it's a clearer and more efficient approach, thanks!
Just for reference, per the GraphQL API docs, CLOSED means that a PR was closed without being merged.
Fixes #3350
PullRequestForBranchwasn't fetching theclosedfield thus makinggh pr reopen <branch>detect a closed PR as already open (pr.Closedzero value = false).Edit:
This PR makes
ghrely only on thestatefield to detect the current PR status (eitherCLOSED,MERGEDorOPEN) (thanks Mislav).