feat: replace PR "Request Changes" with a "API CHANGES REQUESTED" comment #163
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jkleinsc
requested changes
Jul 22, 2024
|
Ya there's a similar discussion happening in the API WG about distinguishing between PR review and API review. I'll mark this as WIP until we reach consensus on the desired behavior. |
itsananderson
changed the title
itsananderson
commented
Aug 9, 2024
| @@ -170,7 +170,7 @@ describe('api review', () => { | |||
| data: [ | |||
| { | |||
| user: { id: 1, login: 'ckerr' }, | |||
| body: 'Fix this please!', | |||
| body: 'API CHANGES REQUESTED', | |||
| state: 'CHANGES_REQUESTED', | |||
There was a problem hiding this comment.
You can use the Request Changes feature in GitHub, but it will only have an impact on API review if you also include API CHANGES REQUESTED in your comment.
|
Updated implementation and PR description to match the new approach we discussed in API WG. PR approvals/change requests are ignored, and instead only specific comment strings are used to approve/decline/request changes to the API. |
jkleinsc
approved these changes
Aug 12, 2024
erickzhao
approved these changes
Aug 12, 2024
codebytere
approved these changes
Aug 13, 2024
codebytere
changed the title
samuelmaddock
approved these changes
Aug 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR: electron/electron#42561 illustrates an odd edge case in our approve/request changes workflow. If you request changes on the PR, the API Review is blocked (even if you were requesting changes to implementation or other non-API parts of the PR). If you subsequently approve the PR, the API Review is still blocked because we don't consider PR approvals to be API approvals.
After some discussion within the API WG, the consensus is that submitting a "Request Changes" to a PR should not be implicitly treated as an API request for changes. Instead, we should add an additional comment match for
API CHANGES REQUESTEDand use that to denote that a reviewer has requested changes to the API.This PR implements this proposal, by removing checks for
REQUEST_CHANGESreviews on the PR, and instead looking forAPI CHANGES REQUESTEDtext in PR review comments.When this lands, there will be 3 comment strings that will change the API Review state:
API LGTM- adds one sign-off for the PR's API changesAPI CHANGES REQUESTED- indicates that changes have been requested for the API. API Review workflow will not be approved while anAPI CHANGES REQUESTEDis active.API DECLINED- indicates that the reviewer doesn't think this API should be added. API Review workflow will not be approved while anAPI DECLINEDis active.As has always been the case, the last review by each API WG member is evaluated by cation, so an
API CHANGES REQUESTEDorAPI DECLINEDcan be "cleared" by adding a follow-upAPI LGTMcomment.