carbon-language/carbon-lang
1
0
Fork 0
mirror of https://github.com/carbon-language/carbon-lang.git synced 2026-09-24 19:50:14 +01:00
Code Issues Packages Projects Releases 10 Wiki Activity

Create code review guidelines (#42)

Browse Source 
Carbon needs a strong code review process to handle code (and other)
changes that are not significant proposals. This attempts to provide
clear guidance on the process, structure, and scope of code review.
It also gives detailed guidance on how to effectively do code review
for both reviewers and authors.

This proposal was accepted on 2020-08-04.
This commit is contained in:
Chandler Carruth
2020-08-27 02:05:40 -07:00
committed by GitHub
parent 9a0bc88bcf
commit 630311c6ae
4 changed files with 616 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CODEOWNERS
+11
View File
@@ -0,0 +1,11 @@
# Global owners.
* @carbon-language/admin-team
# The core team owns the core language and project documentation.
/docs/ @carbon-language/core-team
# The review managers own updates to the proposals.
/proposals/ @carbon-language/review-managers
# This directory contains infrastructure and support scripts
/src/ @carbon-language/infra-team
docs/project/code_review.md
+469
View File
@@ -0,0 +1,469 @@
# Code review
<!--
Part of the Carbon Language project, under the Apache License v2.0 with LLVM
Exceptions. See /LICENSE for license information.
SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-->
## Table of contents
<!-- toc -->
- [High level goals of code review](#high-level-goals-of-code-review)
- [What requires review?](#what-requires-review)
- [Who should review?](#who-should-review)
- [GitHub pull request mechanics](#github-pull-request-mechanics)
- [Code author guide](#code-author-guide)
- [Write good change descriptions](#write-good-change-descriptions)
- [First line](#first-line)
- [Body](#body)
- [Make small changes](#make-small-changes)
- [Responding to review comments](#responding-to-review-comments)
- [Responding to questions or confusion](#responding-to-questions-or-confusion)
- [Understand the feedback in the comments](#understand-the-feedback-in-the-comments)
- [Code reviewer guide](#code-reviewer-guide)
- [How quickly should you respond to a review request?](#how-quickly-should-you-respond-to-a-review-request)
- [What should be covered by a review?](#what-should-be-covered-by-a-review)
- [Writing review comments](#writing-review-comments)
- [Approving the change](#approving-the-change)
- [Resolving an impasse or conflict](#resolving-an-impasse-or-conflict)
- [Escalation](#escalation)
<!-- tocstop -->
## High level goals of code review
Code review serves several goals in the Carbon project. It directly improves the
correctness, clarity, and consistency of the code. These improvements range from
the high-level functionality down through the design and implementation details.
It also promotes team ownership and spreads knowledge across the team.
More detailed discussions can be found in chapter 9 "Code Review" of the book
_[Software Engineering at Google](https://www.amazon.com/Software-Engineering-Google-Lessons-Programming/dp/1492082791)_
and chapter 21 "Collaborative Construction" in
_[Code Complete: A Practical Handbook of Software Construction](https://www.amazon.com/Code-Complete-Practical-Handbook-Construction/dp/0735619670/)_.
However, these details aren't essential to understanding code review and how it
works in the Carbon project. All of the important details are provided in the
project documentation.
## What requires review?
Every change to Carbon's repositories requires code review. Even formal
[evolution decisions](evolution.md) which have been approved should have their
specific changes to the repository reviewed.
Many changes to Carbon repositories may _only_ require code review. Typically,
these include bug fixes, and development or documentation improvements clearly
in line with accepted designs. It may in some rare cases extend to exploring
experimental or prototype directions whose design is under active consideration.
The term "code review" in the Carbon project is not only about "code". We expect
changes to any files to be reviewed, including documentation and any other
material stored in the repository.
## Who should review?
Everyone should feel free to review Carbon changes. Even providing small or
partial review can be a good way to start contributing to Carbon. Contributors
with specific domain expertise or familiarity should also try to provide review
on changes touching relevant parts of the project.
Additionally, at least one _code owner_ of any file changed needs to review that
change. The code owners and what files they are responsible for are defined
using the
[`CODEOWNERS`](https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners#codeowners-syntax)
file in the root of the repository. Pull requests will automatically request
reviewers based on this file and enforce that these reviews take place.
While we do encourage people interested in contributing to Carbon by reviewing
changes to do so, we also suggest not overloading a single review. It can be
daunting for the author of a change to get feedback from a large number of
reviewers, and so we suggest keeping the number of reviewers reasonably small.
Any reviews that explicitly request changes should be addressed, either with the
changes or an explanation of why not, before a pull request is merged. Further,
any code owners who have requested changes should explicitly confirm they're
happy with the resolution before the change is merged.
When a team gives an affirm decision on an [evolution proposal](evolution.md),
each team member should explicitly note any of their comments on the pull
request that, while not blocking the _decision_, still need to be resolved as
part of code review prior to it being merged. These might, for example, be
trivial or minor wording tweaks or improvements. Otherwise, the decision is
assumed to mean the prior review comments from members of that team are
addressed; the author is free to merge once the pull request is approved,
possibly with a code review separate from the proposal's review.
## GitHub pull request mechanics
Carbon uses GitHub pull requests for code review, and we recommend some
mechanical best practices to most effectively navigate them.
- Be aware that the main thread of pull request doesn't support threaded
discussions or "resolving" a comment.
- If either of those would be useful, you'll probably want to comment on a
file.
- You can quote comments in the main conversation thread in a reply by
clicking the three-dot menu on the original comment and selecting "Quote
reply".
- If you will want to comment on files, don't comment in the pull request
conversation.
- Always go to the `Files Changed` tab.
- Make any in-file comments needed, but add them to a pending review
rather than sending them directly.
- Finish the review and add any top-level review comments there.
- If you are a code owner who will be providing approval for the change, then
make sure to mark a review as requesting changes when you want the author to
begin addressing your comment. Only use the "comment" review state if you
are still in the process of reviewing and don't expect the author to begin
working on further changes.
- If you are not a code owner asked to approve, use the difference between
a comment and requesting a change to help the author know whether to
circle back with you before landing the pull request if the relevant
owner(s) approve it.
- Don't reply to in-file comment threads in the conversation view, or with
direct single reply comments.
- Add all replies to in-file comment threads using the `Files Changed` tab
and by adding each reply to a new review, and posting them as a batch
when done.
- You can get to the appropriate `Files Changed` tab by clicking on the
change listed in the conversation view with the incoming set of in-file
comments.
- This flow ensures an explicit update in the overall pull request that
can help both the author and other reviewers note that new replies have
arrived.
- Don't reply to an in-file comment and then mark it as resolved. No one will
see your reply as the thread will be hidden immediately when marked as
resolved.
- Generally, the person who started the comment thread should mark it as
resolved when their comments are sufficiently addressed. If another
reviewer is also on the thread and should also agree, just state that
you're happy and the last reviewer can mark it resolved.
- Trivially resolved threads can just be marked as "resolved" without
further update. Examples: a suggested change that has been successfully
applied, or a thread where the relevant reviewers have clearly indicated
they're happy.
## Code author guide
The goal of an author should be to ensure their change improves the overall
code, repository, and/or project. Within the context of code review, the goal is
to get a reviewer to validate that the change succeeds at this goal. That
involves finding an effective reviewer given the particular nature of the
change, helping them understand the change fully, and addressing any feedback
they provide.
### Write good change descriptions
The change description in the pull request is the first thing your reviewers
will see. This sets the context for the entire review, and is very important.
#### First line
The first line of a commit, or the subject of the pull request, should be a
short summary of specifically what is being done by that change. It should be a
complete sentence, written as though it was an order. Try to keep it short,
focused, and to the point.
#### Body
The description body may need to explain several important aspects of the change
to provide context for the reviewer when it isn't obvious from the change
itself:
- The problem being solved by the change.
- Why the approach taken is the best one.
- Any issues, concerns, or shortcomings of the approach.
- Any alternatives considered or attempted.
- Relevant supporting data such as examples or benchmarks.
Try to anticipate what information the reviewer of your change will need to have
in order to be effective. Also consider what information someone else will need
a year in the future when doing archaeology on the codebase and they come across
your change without any context.
### Make small changes
Small changes have many benefits:
- Faster review.
- More thorough review.
- Easier to merge.
- Easier to revert if needed.
The ideal size of a change is as small as possible while it remains
self-contained. It should address only _one thing_. Often, this results in a
change only addressing _part_ of a feature rather than the whole thing at once.
This makes work more incremental, letting the reviewer understand it piece by
piece. It can also make it much easier to critically evaluate whether each part
of a feature is adequately tested by showing it in isolation.
That said, a change should not be so small that its implications cannot easily
be understood. It is fine to provide the reviewer context or a framework of a
series of changes so they understand the big picture, but that will only go so
far. It is still possible to shrink a change so much that it becomes nonsensical
in isolation. For example, a change without appropriate tests is not
self-contained.
You may want to use a set of stacked pull requests rather than a single, larger
pull request in order to keep changes easy to review.
> TODO: link to the stacked pull request documentation when available.
### Responding to review comments
Many comments have easy and simple responses. The easiest is **"Done"**. When
the comment is a concrete suggestion that makes sense and you implement it, you
can simply let the reviewer know their suggestion has been incorporated. If the
_way_ you implemented the suggestion might need clarification, add that as well.
For example, consider mentioning tweaks to the suggestion or when the suggestion
was applied in more places.
When a suggestion from the reviewer is explicitly optional, you may also have a
simple response that you're not going to make the change. This is totally fine
-- if it weren't, the reviewer shouldn't have listed it as optional -- but it
may be helpful to explain your reasoning to the reviewer so they understand
better why the optional suggestion didn't make sense to you.
Sometimes comments, even optional ones, center around slight differences or
preferences around the code. Consider that the reviewer may be a good proxy for
future readers. If the suggestion is essentially equivalent to your original
code, consider adopting it as it may make the code easier to read for others.
But if you feel the current choice is _better_, even if only slightly, stand up
for yourself and keep it. The reviewer can always push for a change and justify
it if needed.
For non-optional comments, this section provides several suggestions on how best
to make progress. If none of these work, you may need to
[resolve an impasse or conflict](#resolving-an-impasse-or-conflict).
#### Responding to questions or confusion
Some comments in code review will be questions or confusion as the reviewer
tries to understand the code in question or why a particular approach was used.
Don't assume that questions are a request for a change. Reviewers should be
explicit if they think a change is needed rather than merely asking questions.
You should assume a question or confusion is something which only needs to be
clarified.
However, when responding to a question or confusion, consider making changes to
improve clarity in addition to responding within the review, such as by adding
comments or changing code structure. The reviewer may not be the last person to
need more clarity, and you should use their comments as a signal for
improvement. Once done, the review response should typically focus on verifying
that the clarifications made in the code are sufficient for the reviewer.
#### Understand the feedback in the comments
At times, review comments may be confusing or frustrating for you. While this is
something we always want reviewers to minimize, it will still happen at some
times and to some degree. It helps to remember that the goal of the review is to
ensure the change results in the project improving over time.
If the review comment doesn't make sense, ask the reviewer to help you
understand the feedback better. If it isn't constructive or doesn't seem to
provide any meaningful path forward, ask the reviewer to provide this. Making
comments both clear and constructive are part of the reviewers'
responsibilities.
Once there is a clear and effectively communicated comment that you understand,
it may still feel wrong or like it is unnecessarily blocking your progress. It
is important to try to step back in this situation and, no matter how certain
you are, genuinely consider whether there is valuable feedback. You should be
asking yourself whether the reviewer might be correct, potentially in an
unexpected or surprising way. If you can't decide this definitively, you may
need to work to get a deeper understanding.
If you are confident that the reviewer's comment is incorrect, that is _OK_. The
reviewer is also only human and is certain to make mistakes and miss things. The
response needs to try to explain what it is that leads you to be confident in
your assessment. Lay out the information you have and how you are reasoning
about the issue to arrive at the conclusion. Try not to make assumptions about
what the reviewer knows or why they made the comment. Instead, focus on
surfacing explicitly your perspective on the issue.
These parts of a review will often be a discussion and may need to iterate a few
times. That isn't intrinsically bad, but try to make sure that it doesn't result
in reiterating positions or repeating things. Make sure the discussion is
_progressing_ towards deeper understanding and recognize when you reach an
impasse or conflict and shift strategy to
[resolve that](#resolving-an-impasse-or-conflict). It is also useful to avoid
long delays between these iterations. Consider discussing over Discord chat or
scheduling a quick video chat on the specific issue. This can avoid multi-hour
-- or multi-day -- round trips.
## Code reviewer guide
The specific goal for a particular review should always be to ensure that the
overall health of the code, repository, and/or project improves over time. This
requires that contributions _make progress_ -- otherwise, nothing can improve.
However, the review should ensure that quality of changes does not cause the
health of the project to decrease over time.
The primary responsibility for ensuring that code review remains constructive,
productive, and helpful resides in the _reviewer_. As a reviewer, you are in a
position of power and asked to critique the authors hard work. With this power
comes responsibility for conducting the review well.
### How quickly should you respond to a review request?
Try to respond to code review requests as soon as you can without interrupting a
focused task. At the latest, the next day you are working on the project. Note
that the review isn't expected to necessarily be complete after a single review.
It is more valuable to give reasonably quick but partial feedback than to delay
feedback in order to complete it. If leaving partial feedback, make it clear to
the author which parts are covered and which you haven't gotten to yet.
Large changes are especially important to give incremental feedback on in order
to do so in a timely fashion. One of the first things to consider with large
changes is whether it can be split apart into smaller changes that are easier to
review promptly.
This timeliness guidance doesn't apply to the higher-level
[evolution process](evolution.md) reviews. Evaluating those proposals will often
require a larger time investment and have their own timelines spelled out in the
process. Here, we are talking about simply reviewing changes themselves
orthogonally to any evolutionary discussion and evaluation.
### What should be covered by a review?
Things to consider and evaluate when reviewing changes:
- Is the code well designed?
- Is the resulting functionality, including its interface, good for the
users of the code?
- Does the resulting design facilitate long-term maintenance?
- Can the code be simplified? Is there unnecessary complexity?
- Are things being implemented that aren't yet needed and only _might_ be
needed in the future?
- Is the code free of bugs and well tested?
- Is memory safely managed?
- Is any parallel or concurrent programming done safely?
- Do unit tests cover relevant behaviors and edge cases?
- Do any integration tests need to be extended or added?
- Do any fuzz tests need to be extended or added?
- Are any tests well designed to be both thorough but also maintainable
over time?
- Is the code easy to read?
- Are the names used in the code clear?
- Are all important or non-obvious aspects of the code well commented? Do
the comments focus on _why_ instead of _what_?
- Is there appropriate high level documentation for the change?
- Does the change adhere to all relevant style guides?
- Is the change consistent with other parts of the project?
### Writing review comments
These are general guidelines for writing effective code review comments:
- **Be kind.** Detailed review, especially in an open source project, can be
stressful and difficult for the author. As a reviewer, part of the job is to
ensure the review experience ends up positive and constructive for the
author.
- **Stay constructive.** Focus your comments on suggesting specific ways to
improve the change. If you need to explain why an improvement is necessary,
focus on objective ways the improvement helps and avoid both subjective
assessments and anchoring on problems with the current state.
- **Explain why.** It is important for the author to understand not merely the
mechanical suggested change but what motivates it and why it matters. This
may help clear up misunderstandings, help the suggestion be understood and
applied more effectively, and allow internalizing improvements for future
contributions.
- **Provide a path forward.** The author needs to understand what they will
need to do to respond to your comments. For example, always provide
alternatives when commenting that the current approach won't work.
Keep in mind that the goal is to improve the overall health of the code,
repository, and/or project over time. Sometimes, there will be pushback on
review comments. Consider carefully if the author is correct -- they may be
closer to the technical issues than you are and may have important insight. Also
consider whether the suggestion is necessary to achieve the overall goal. If the
suggestion isn't critical to make the change an overall improvement, it may be
fine for it to move forward as-is.
As with all communication in the Carbon project, it is critical that your
comments are not unkind, unwelcoming, angry, ad-hominem attacks, or otherwise
violating our community's [code of conduct](/CODE_OF_CONDUCT.md).
### Approving the change
Be explicit and unambiguous at the end of your review. Select "Approve" when
submitting the review to mark this in GitHub. You can always include a message,
often "LGTM" or "Looks Good To Me" is often used. If you don't feel like you're
in a position to approve the change and are simply helping out with review
feedback, make that explicit as well. You should set the review to a "Comment"
in GitHub, but also state this explicitly in the message since this is the
default and doesn't indicate that your feedback _is_ addressed. For example, say
that "my comments are addressed, but leaving the final review to others" to
clearly indicate that you're happy but are deferring the decision to others. If
you are a code owner and deferring to someone else, it is essential to suggest
specific other reviewers. Otherwise, we risk all the code owners assuming
another is going to approve the change.
An important technique to make progress, especially with different working hours
and timezones, is to approve changes even with outstanding comments. For
example, if the comments you have are straightforward and have unambiguous fixes
or suggested edits, you should give an LGTM with those comments addressed. The
author can always come back to you if they have questions, and we can always
revert changes if the resolution for some reason diverges wildly from your
expectations.
## Resolving an impasse or conflict
At some point, a review may reach an impasse or a genuine conflict. While our
goal is always to resolve these by building consensus in review, it may not be
possible. Both the author and any reviewers should be careful to recognize when
this point arrives and address it directly. Continuing the review is unlikely to
be productive and has a high risk of becoming acrimonious or worse.
There are two techniques to use to resolve these situations that should be tried
early on:
1. Bring another person into the review to help address the specific issue.
Typically they should at least be a code owner, and may usefully be a member
of the [core team](groups.md#core-team).
2. Ask the specific question in a broader forum, such as Discord Chat, in order
to get a broad set of perspectives on a particular area or issue.
The goal of these steps isn't to override the author or the reviewer, but to get
more perspectives and voices involved. Often this will clarify the issue and its
trade-offs, and provide a simple resolution that all parties are happy with.
However, in some cases, the underlying conflict isn't actually addressed. While
there is a desire to generally bias towards the direction of the code owners
during reviews, reviews should _not_ turn into a voting process. The reason for
proceeding in a specific direction should always be explained sufficiently that
all parties on the review are satisfied by the explanation and don't feel the
need to escalate.
Fundamentally, both reviewers and the author need to agree on the direction to
move forward. If reaching that agreement proves impossible, the review should be
[escalated](#escalation). If you feel like an escalation is needed in a review,
be explicit and clear in requesting it. There is nothing bad about going through
this process, but it should only occur when needed and so it helps to be very
clear.
Once the impasse or conflict is addressed, it is _essential_ to commit to that
direction. It can be especially difficult for the author to accept a direction
that they initially disagree with and make changes to their code as a result. An
essential skill is the ability to
[disagree and commit](https://en.wikipedia.org/wiki/Disagree_and_commit).
## Escalation
At the explicit request of any member of the [core team](evolution.md#core-team)
or to resolve any fundamental impasse in a review, the change should move to the
formal [evolution process](evolution.md#evolution-process). Ultimately, the
Carbon project [governance](evolution.md#governance-structure) structure is
always available as an escalation path.
Before escalating an impasse or conflict in code review, try asking another
reviewer to help resolve the issue or bridge any communication gaps. Consider
scheduling a quick video chat to discuss and better understand each others'
concerns and position.
Note that the formal evolution process is heavyweight and relatively slow. The
expectation is that this is rarely used and only to resolve serious and severe
disagreements. If this becomes a more common problem, lighter weight processes
may be needed to help ensure a reasonable rate of progress.
proposals/README.md
+1
View File
@@ -25,6 +25,7 @@ request:
- [0029 - Linear, rebase, and pull-request GitHub workflow](p0029.md)
- [Decision](p0029_decision.md)
- [0042 - Create code review guidelines](p0042.md)
- [0044 - Proposal tracking](p0044.md)
- [Decision](p0044_decision.md)
- [0051 - Goals](p0051.md)
proposals/p0042.md
+135
View File
@@ -0,0 +1,135 @@
# Create code review guidelines
<!--
Part of the Carbon Language project, under the Apache License v2.0 with LLVM
Exceptions. See /LICENSE for license information.
SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-->
[Pull request](https://github.com/carbon-language/carbon-lang/pull/42)
## Problem
Carbon should ensure that all checked-in changes to the repository are properly
code reviewed, and that the process for code review is effective across a number
of dimensions:
- Ensure high quality of code, documentation, and other artifacts. We consider
these all "code reviews" regardless of whether the final artefact is "code"
in a technical sense.
- Encourage broad participation and contribution to the community through code
reviews.
- Ensure code reviews are inclusive, respectful, and welcoming.
- Have clear, discoverable, and mechanically enforced (where possible) rules
for who _can_, who _should_, and who _must_ review any particular change.
## Background
General code review:
- Chapter 9 "Code Review" in
_[Software Engineering at Google](https://www.amazon.com/Software-Engineering-Google-Lessons-Programming/dp/1492082791)_
- Chapter 21 "Collaborative Construction" in
_[Code Complete: A Practical Handbook of Software Construction](https://www.amazon.com/Code-Complete-Practical-Handbook-Construction/dp/0735619670/)_
- [Respectful Code Reviews (Chromium)](https://chromium.googlesource.com/chromium/src/+/master/docs/cr_respect.md)
- [Compassionate--Yet Candid--Code Reviews (video)](https://youtu.be/Ea8EiIPZvh0)
- [The Standard of Code Review](https://google.github.io/eng-practices/review/reviewer/standard.html)
- [How We Do Code Review](https://devblogs.microsoft.com/appcenter/how-the-visual-studio-mobile-center-team-does-code-review/)
- [Code Reviewing in the Trenches: Understanding Challenges, Best Practices and Tool Needs](https://www.microsoft.com/en-us/research/wp-content/uploads/2016/05/MS-Code-Review-Tech-Report-MSR-TR-2016-27.pdf)
- [The Importance of Code Reviews](https://www.sitepoint.com/the-importance-of-code-reviews/)
- [10 Reasons Why Code Reviews Make Better Code and Better Teams](https://simpleprogrammer.com/why-code-reviews-make-better-code-teams/)
- [Wikipedia article on code review](https://en.wikipedia.org/wiki/Code_review)
- [Expectations, Outcomes, and Challenges of Modern Code Review](https://sback.it/publications/icse2013.pdf)
Specific GitHub tooling:
- [About code owners](https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners)
- [Reviewing changes in pull requests](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/reviewing-changes-in-pull-requests)
## Proposal
Add a [code review guide](/docs/project/code_review.md) to the project, and
reference it from our contributing guide and pull request workflow
documentation.
Also create initial `CODEOWNERS` files in both this repository and the toolchain
repository based on current review activity and team roles. These are really
only suggested as an initial guess and should likely be iterated frequently as
more people join and begin contributing. The `carbon-lang` repository file is
included directly, and carbon-language/carbon-toolchain#1 updates the
`carbon-toolchain` repository.
## Alternatives considered
### Post-commit review
Some projects, such as LLVM, use _post-commit_ review for some changes. This has
both pros and cons.
Pros:
- Enables sustaining velocity in the face of high latency in code review.
- Little need to rebase or resolve merge conflicts due to immediately
landing patches even while under review.
- Cross-developer dependencies don't create challenges.
- Optimizes commit velocity amongst a small group of developers with extensive
experience working together and minimal review comments on others' code.
Cons:
- Does not meaningfully scale beyond small group of developers with
pre-existing shared understanding of desired form of code.
- Relies on relative infrequency of needing code review comments.
- Developers largely need to be well aligned and reliably writing code
others would already approve.
- Disincentivizes code review relative to writing code.
- This effect is strong enough to create a significant fraction of commits
that simply see no review in the LLVM community.
- Creates significant barriers for new contributors.
- In practice, existing contributors are much more likely to be able to
accurately create post-commit review passing changes than new
contributors.
- Because of this, new contributors will have a very difficult time
joining the community.
- This has specifically been cited by people joining the LLVM community.
### Skipping review when no functionality is changed (NFC commits)
Another practice popular in the LLVM community is to skip pre-commit review for
changes for which "no functionality changes" or NFC commits. Common examples are
reformatting or basic code cleanup. The idea is that these are exceedingly lower
risk compared changes to functionality.
Pros:
- Can avoid waiting for a review on trivial cleanups and refactorings.
- May be especially useful as they tend to be merge conflict prone and
likely to be lead-ups to changes sent out for review.
- The advantage is lessened when using stacked reviews to parallelize
them.
- Avoid spending reviewer time on more trivial changes.
- Unclear how much time this is as NFC changes are typically relatively
fast to review due to their nature. Deciding "did it change behavior?"
is easier than deciding "given that it changes behavior, is the change
good?".
Cons:
- No concrete and objective rubric for whether or not a change is NFC, and
whether it perhaps is significant enough to still warrant review.
- Debating this adds a new cost to the entire process.
- In some cases, deciding whether a change is NFC roughly requires a code
review, making it pointless to skip the code review.
- A specific utility of code review is to discover when something
_unexpected_ happens. The fact that the author _believes_ a change is
NFC doesn't address this utility as the unexpected thing may be the
functionality changed.
- Loses the knowledge sharing benefit of code review for cleanups and
refactorings.
- Especially unfortunate as these are exactly the kinds of changes often
recommend for people starting to get familiar with a project.
- Fails to ensure consistency or ease of understanding of the code.
- Despite not changing functionality, a change may decrease how easily
understood the code is or may move it to be inconsistent with the wider
codebase.
- Avoiding these two things are some of the primary goals of code review.