Skip to content

Commit

Permalink
Merge branch 'master' into patch-2
Browse files Browse the repository at this point in the history
  • Loading branch information
adambender authored Jul 31, 2020
2 parents 3a7ba63 + c203258 commit 5c8e8a0
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 19 deletions.
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ Currently this contains the following documents:
There is some Google-internal terminology used in some of these documents, which
we clarify here for external readers:

* **CL**: Stands for "changelist," which means one self-contained change that
* **CL**: Stands for "changelist", which means one self-contained change that
has been submitted to version control or which is undergoing code review.
Other organizations often call this a "change", "patch", or a "pull request (PR)".
* **LGTM**: Means "Looks Good to Me." It is what a code reviewer says when
Other organizations often call this a "change", "patch", or "pull-request".
* **LGTM**: Means "Looks Good to Me". It is what a code reviewer says when
approving a CL.

## License
Expand Down
31 changes: 21 additions & 10 deletions review/developer/small-cls.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,16 +114,27 @@ more difficult if included in your current CL.

## Keep related test code in the same CL {#test_code}

Avoid splitting test code into a separate CL. Tests validating your code
modifications should go into the same CL, even if it increases the code line
count.

However, <i>independent</i> test modifications can go into separate CLs first,
similar to the [refactorings guidelines](#refactoring). That includes:

* validating pre-existing, submitted code with new tests.
* refactoring the test code (e.g. introduce helper functions).
* introducing larger test framework code (e.g. an integration test).
CLs should include related test code. Remember that [smallness](#what_is_small)
here refers the conceptual idea that the CL should be focused and is not a
simplistic function on line count.

A CL that adds or changes logic should be accompanied by new or updated tests
for the new behavior. Pure refactoring CLs (that aren't intended to change
behavior) should also be covered by tests; ideally, these tests already exist,
but if they don't, you should add them.

_Independent_ test modifications can go into separate CLs first, similar to the
[refactorings guidelines](#refactoring). That includes:

* Validating pre-existing, submitted code with new tests.
* Ensures that important logic is covered by tests.
* Increases confidence in subsequent refactorings on affected code. For
example, if you want to refactor code that isn't already covered by
tests, submitting test CLs _before_ submitting refactoring CLs can
validate that the tested behavior is unchanged before and after the
refactoring.
* Refactoring the test code (e.g. introduce helper functions).
* Introducing larger test framework code (e.g. an integration test).

## Don't Break the Build {#break}

Expand Down
12 changes: 6 additions & 6 deletions review/reviewer/comments.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@
## Courtesy

In general, it is important to be
courteous and respectful while also being
very clear and helpful to the developer whose code you are reviewing. One way to
do this is to be sure that you are always making comments about the *code* and
never making comments about the *developer*. You don't always have to follow
this practice, but you should definitely use it when saying something that might
otherwise be upsetting or contentious. For example:
[courteous and respectful](https://chromium.googlesource.com/chromium/src/+/master/docs/cr_respect.md)
while also being very clear and helpful to the developer whose code you are
reviewing. One way to do this is to be sure that you are always making comments
about the *code* and never making comments about the *developer*. You don't
always have to follow this practice, but you should definitely use it when
saying something that might otherwise be upsetting or contentious. For example:

Bad: "Why did **you** use threads here when there's obviously no benefit to be
gained from concurrency?"
Expand Down

0 comments on commit 5c8e8a0

Please sign in to comment.