google author guide - to getting through code review

Writing Good CL Descriptions

• 설명은, 나중에 다른 개발자들의 PR 검색으로 쓰인다. 이 부분을 고려하자.

First line

• 요약
  • 뭘 했는지 짧게 요약
  • 명령인 것처럼 쓰여진 완전한 문장이어야 함
  • 첫 줄 후에 empty line 이 와야한다..
• 이것은 버전 제어 기록 요약에 나타나는 내용
• 향후 코드 검색자가 CL이 실제로 수행한 작업이나 다른 CL과 어떻게 다른지 이해하기 위해, 유용해야 한다.

Body is informative

• 해결 중인 문제 & 이 방법이 최선의 방법인 이유 & 독자가 변경 목록을 전체적으로 이해하는 데 필요한 보충 정보
• 접근 방식에 어떤 단점이 있다면, 그것들을 언급해야 한다.

Good CL Descriptions ?

Functionality change

• 처음 몇 단어는 CL이 실제로 하는 일을 설명합니다.
• 나머지 설명에서는 해결 중인 문제, 왜 이것이 좋은 해결책인지, 그리고 구체적인 구현에 대한 좀 더 많은 정보에 대해 설명합니다.

refactoring

• 첫 번째 줄은 CL이 하는 일과 이것이 과거와 어떻게 달라졌는지를 설명합니다.
• 설명의 나머지 부분에서는 솔루션이 이상적이지 않다는 구체적인 구현, CL의 맥락 및 향후 방향에 대해 설명합니다.
• 그것은 또한 왜 이러한 변화가 이루어지는지 설명한다.

Small CL that needs some context

• 첫 번째 문장은 실제로 수행되고 있는 것을 설명합니다.
• 설명의 나머지 부분은 변경이 이루어지는 이유를 설명하고, 검토자에게 많은 맥락을 제공합니다.

Review the description before submitting the CL

• CL은 검토 중에 중요한 변화를 겪을 수 있다.
• CL을 제출하기 전에 CL 설명을 검토하여, 설명이 CL이 수행하는 작업을 반영하는지 확인하는 것이 좋습니다.

---

Small CLs

왜 small CLs를 작성해야 하는가?

• reviewer은 변경 내용이 너무 크다는 이유만으로, 변경 내용을 전면 거부할 수 있다.
• 왜 작게 작성해야해?
  • 더 빨리 검토할 수 있다
  • 더 철저하게 검토할 수 있다
  • 버그가 발생할 가능성이 적다.
  • 잘못된 방향에서 빠르게 빠져나올 수 있다
  • rebase & merge 가 쉽습니다
  • 설계가 더 용이합니다
  • roll back이 더 간단합니다

what is small?

• CL은 한 가지만 다루는 최소한의 변경을 수행합니다.
• 테스트 코드를 포함해야 합니다.
• reviewer가 이해할 수 있도록 충분한 설명을 제공해야 합니다.
• merge 된 이후에도 잘 작동해야 한다.
• 너무 작아서, 이해가 안 될 정도면 안된다.

when are Large CLs Okay?

• 전체 파일 삭제 등
• lint / yapf 와 같은 신뢰받는 자동 리펙토링 도구 사용

Separte Out Refactorings

• 버그 수정과 리펙토링은 다른 CL에 있어야 이상적이다.
• 하지만, 변수 이름 수정과 같은 작은 정리는 포함될 수 있다.

Keep related test code in the same CL

• 테스트 코드 도입 후, 읽어볼 계획

CLs should include related test code. Remember that smallness 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. 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).

충분히 작게 만들 수 없다?

• 때때로 당신은 당신의 CL이 커야 하는 것처럼 보이는 상황에 직면할 것이다. 하지만, 이것은 거의 사실이 아니다.
• 작은 CL을 작성하는 것을 연습하는 저자들은 기능성을 일련의 작은 변화로 분해하는 방법을 거의 항상 찾을 수 있다.
• 대규모 CL을 작성하기 전에 리팩터링 전용 CL로 선행하는 것이 더 깨끗한 구현을 위한 길을 닦을 수 있는지 고려해라.
• 이러한 모든 옵션이 실패하는 경우(매우 드물어야 함) 사전에 검토자의 동의를 얻어 대규모 CL을 검토하여 향후 발생할 내용에 대해 경고를 받도록 해라.

---

How to Handle Reviewer Comments

Don`t Take it personally

• 리뷰 커멘트를, Author에 대한 공격이 아니라, 코드 베이스 및 서비스를 돕기 위한 시도라고 생각하라.
• 항상 "검토자가 나에게 전달하려고 하는 건설적인 것은 무엇인가?"를 생각하자.
  • 이 질문에 답이 안나오면, reviewer에게 설명을 요청하라.
  • 예의와 존중이 최우선이다.

Fix the code

• 검토자가 코드의 내용을 이해하지 못하겠다고 말하는 경우
  • 가장 먼저 해야 할 일은 코드 자체를 명확히 수정하는 일
  • 두번째는, 코드 주석 추가이다.
• 검토자가 이해하지 못했다는 것은, 다른 사용자도 이해하기 어렵다는 뜻이 될 수 있다.