Skip to content

Updated code review guidelines #376

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 4, 2024
Merged

Updated code review guidelines #376

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
20c1eb4
Updated code review guidelines.
trmartin4 Jun 29, 2024
3f8aec1
Updated code review and pull request from feedback
trmartin4 Jul 3, 2024
247aa75
Merge branch 'main' into update-review-process
trmartin4 Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion docs/contributing/pull-requests/code-review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,17 @@ claims. This can be based on testing the change or on previous domain knowledge.
- Solve the intended problem,
- [solve the requirements in the best way](#assumptions-note),
- the code is well structured,
- follows our most recent, accepted patterns,
- follows our most recent, accepted patterns as defined in our [ADRs](../../architecture/adr/),
- and is free of unintended side-effects.

:::warning Evolutionary Database Design

For any database changes, be sure that they follow
[EDD](../../contributing/database-migrations/edd.mdx). This is important as the lack of EDD support
will **not** be caught by any unit, integration, or regression testing.

:::

If you are unsure about any of the above, consider using a different status or check in with the
author to discuss things first. Also don’t hesitate to request a second review from someone else.

Expand Down
42 changes: 21 additions & 21 deletions docs/contributing/pull-requests/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,33 +115,16 @@ These reviews are required for the changes to be merged to `master`.
You can tag additional teams or individuals for review as you see fit, including `@dept-design` for
any design changes.

</Bitwarden>

### Opening the PR for review

As its name implies, marking a PR as "Ready for Review" indicates that you are ready for all
assigned teams to review it. If the changes are still in progress, leave the PR in `Draft` status.
Doing this ensures that reviewers can act on the "Ready for Review" as their signal to begin the
review process without further notification.

### Addressing feedback

It is likely that you will receive some feedback on your PR. You should see this as a positive thing
-- it signifies a healthy and thorough review process and an organizational commitment to code
quality. You may receive [comments](./code-review.md#comment) or a
[request for changes](./code-review.md#request-changes). You are encouraged to engage in
conversation on the PR to discuss a solution, but if any strong conflicting opinions arise it is
often best to move the conversation to a synchronous format to avoid any misunderstanding.

When any necessary changes have been made, you should address the comments or request for changes by
responding in the PR conversation thread. You are not responsible for resolving the conversation --
that is the prerogative of the reviewer, to ensure that they agree that the question or concern has
been addressed.

**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.

</Bitwarden>

## Reviewing the pull request
## Managing the review process
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


<Bitwarden>

Expand Down Expand Up @@ -199,7 +182,24 @@ that may delay this process.

</Community>

### How to perform a review
### Addressing feedback

It is likely that you will receive some feedback on your PR. You should see this as a positive thing
-- it signifies a healthy and thorough review process and an organizational commitment to code
quality. You may receive [comments](./code-review.md#comment) or a
[request for changes](./code-review.md#request-changes). You are encouraged to engage in
conversation on the PR to discuss a solution, but if any strong conflicting opinions arise it is
often best to move the conversation to a synchronous format to avoid any misunderstanding.

When any necessary changes have been made, you should address the comments or request for changes by
responding in the PR conversation thread. You are not responsible for resolving the conversation --
that is the prerogative of the reviewer, to ensure that they agree that the question or concern has
been addressed.

**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
Copy link
Member

@eliykat eliykat Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, Github is showing additional lines as context which is confusing. This is referring to the Performing a review section.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this now, thank you. I've added a callout in 3f8aec1 (#376)


We've written up some [guidelines](./code-review.md) for reviewing code, which we recommend reading
before performing your first code review.
Expand Down