-
Notifications
You must be signed in to change notification settings - Fork 50
Add Reviewer Guidance: Comment/Approve/ Request Changes. #639
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
base: arm-software
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,31 @@ If the issue is in Arm Toolchain itself, or if you are not sure, please create a | |
| Please attach any support files to the issue itself, | ||
| see [Attaching files](https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/attaching-files). | ||
|
|
||
| ### Reviewing Patches | ||
|
|
||
| All contributions to Arm Toolchain are reviewed through GitHub Pull Requests. | ||
| If you are asked to review a patch, or would like to understand the review process, please follow the guidance below. | ||
|
|
||
| How to Review a Pull Request | ||
|
|
||
| When reviewing a PR, you can provide feedback, approve the PR (if you have the required permissions), or request changes | ||
| using GitHub’s standard review options: | ||
| * Comment – leave feedback without blocking the merge. | ||
| * Approve – Approve merging the changes proposed in the PR provided you have the necessary permissions. | ||
| * Request changes – If a reviewer selects this option, the identified issues must be fixed before the PR can move forward. | ||
| The reviewer who requested changes should re-review the PR after updates are made. The PR should not be merged until the concerns | ||
| are resolved and the review is either approved or dismissed. If the original reviewer is unavailable, another authorized reviewer may approve or dismiss a blocking review if needed with proper justification to allow the PR to merge. | ||
|
|
||
| Resolving review conversations | ||
|
|
||
| The author may directly resolve minor comments such as nits, formatting adjustments or other trivial issues. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be confusing as "resolve comment" may mean fix the code, while here this specifically refers to click Resolve button on a comment. Also, you may try to rephrase by starting with why, e.g. along the lines of In order to allow reviewers to remember the context of requested change, do not resolve the conversation, but comment how it was fixed. then go on to say that minor changes that are accepted as requested can be resolved by the author. |
||
| For substantive comments, the author should notify the reviewer after making the required changes. The reviewer | ||
| should provide explict confirmation or perform a re-review before making the conversation as resolved. This ensures | ||
| that all meaningful feedback is properly addressed and prevents premature closure of important discussions. | ||
|
|
||
| For full guidance on how to review patches and use these options, please refer to GitHub’s documentation: | ||
| see [Reviewing Proposed Changes in a PR](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request) | ||
|
|
||
| ### Submit a fix | ||
|
|
||
| For a small change, please create a Pull Request as described in | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a context for these changes? Otherwise it seems like stating the obvious. Is this for engineers who have not worked with Github before?