Avoid updating state on closed PRs #270
Conversation
derekjobst
changed the title
pull/context.go
Outdated
| // State returns the state of the pull request (open or closed) | ||
| State() string |
There was a problem hiding this comment.
Since this is either open or closed is there an advantage to returning it as a string instead of implementing it as an IsClosed() or IsOpen() method?
There was a problem hiding this comment.
Returning a bool seems like a better option. Changed.
I believe the values are limited to open and closed, although I thought I saw somewhere that merged was also an option. After looking again, this is the best enumeration of state values I've been able to find and it's just the two.
|
|
||
| // IsClosed returns true when the state of the pull request is "closed" | ||
| IsClosed() bool | ||
|
|
There was a problem hiding this comment.
Perhaps IsClosed is not necessary yet, but it felt right to add since open and closed are the two possible values (that I am aware of, right now).
| @@ -88,6 +88,11 @@ func (b *Base) PostStatus(ctx context.Context, prctx pull.Context, client *githu | |||
| sha := prctx.HeadSHA() | |||
| base, _ := prctx.Branches() | |||
|
|
|||
| if !prctx.IsOpen() { | |||
There was a problem hiding this comment.
!IsOpen seemed better than IsClosed since there could be additional options added in the future. We are specifically looking to skip in all cases except when status is open, rather than just when it is closed.
derekjobst
deleted the
djobst/prevent-detail-page-status-updates-for-merged-prs
branch
Addresses #269