Updated code review guidelines #376
Conversation
|
No New Or Fixed Issues Found |
| </Bitwarden> | ||
|
|
||
| ## Reviewing the pull request | ||
| ## Managing the review process |
There was a problem hiding this comment.
I suggest this section is moved to or integrated with code-review.md. It describes the expectations the company has about the review process, including expectations of the reviewer, whereas the rest of this page is more of a how-to guide for the PR author. This change in tone and focus is probably the source of the confusion.
You could mention it briefly here, like "You should receive a review or at least first contact from the reviewer within 2 business days of marking the PR as ready for review. If you don't hear anything within this time, it is acceptable (and expected) that you reach out to the reviewer via Slack." Then you could have a callout, e.g. "See [Code Review] for more information about the code review process."
There was a problem hiding this comment.
I agree it's confusing, and I've struggled with the separation between the "Pull Requests" and "Code Review" content for a while, and with each change I make in this area.
Do you think it would be more helpful to extract all of the review content (from the author and the reviewer perspective) into the "Code Review" page? It feels like the author / reviewer separation might be artificial and make the documentation confusing.
There was a problem hiding this comment.
I think that would also work - this page steps you through the process of branching, committing, and submitting the PR, and then you direct the reader to the Code Review page for next steps.
There was a problem hiding this comment.
I've taken your suggestion to extract that section into the "Code Review" page. I think it reads much more clearly now - thank you so much for the suggestion!
| **When you are ready for a reviewer to revisit your changes, you should request a re-review.** This | ||
| will notify the reviewer and ensure a prompt response. | ||
|
|
||
| ## Performing a review |
There was a problem hiding this comment.
This also doesn't fit in with the rest of the page, I recommend it's moved to a callout, maybe at the top of the page. That keeps this page wholly directed at the PR author.
There was a problem hiding this comment.
This is directed at the author, is it not? This is telling the PR author to re-request reviews from their reviewers when they have made all requested changes.
There was a problem hiding this comment.
Sorry, Github is showing additional lines as context which is confusing. This is referring to the Performing a review section.
There was a problem hiding this comment.
I see this now, thank you. I've added a callout in 3f8aec1 (#376)
Deploying contributing-docs with
|
| Latest commit: |
247aa75
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b56a361a.contributing-docs.pages.dev |
| Branch Preview URL: | https://update-review-process.contributing-docs.pages.dev |
🎟️ Tracking
Feedback from new engineers on the team that the flow of the PR / Code Review section didn't make sense.
📔 Objective
The "Pull Requests" page is designed to describe the process and expectations of the PR author. The section titled "Reviewing the pull request" was misleading, as it implied that it transitioned the point of view to that of the reviewer. That is not the case, and caused some confusion for new readers. I've restructured that page to be more clear.
In addition, I added a callout for the PR reviewer to make sure that we emphasize our adherence to EDD and the responsibility of the reviewer to check that.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes