작은 CL
왜 작은 CL을 작성해야 하나요?
작고 단순한 CL은 다음과 같은 특징을 갖습니다:
- 더 빠르게 리뷰됩니다. 리뷰어가 작은 CL을 5분씩 여러번 리뷰하는 일이 큰 CL을 리뷰하기 위해서 30분을 따로 빼두는 것보다 쉽습니다.
- 더 철저하게 리뷰됩니다. 수정 사항이 크면, 리뷰어와 저자는 많은 양의 자세한 설명 때문에 왔다갔다 해야해서 좌절하게 됩니다. 가끔은 중요한 부분을 놓치거나 유야무야하게 됩니다.
- 버그가 생길 가능성이 줄어듭니다. 작은 수정을 하고 있기 때문에, 당신과 당신의 리뷰어는 CL이 어떤 영향을 끼치게 되고 어떤 버그가 생기게 될지 좀더 효과적으로 추론할 수 있습니다.
- 반려되어도 일이 덜 낭비됩니다. 커다란 CL을 작성했는데 리뷰어가 전체적인 방향이 틀렸다고 해버리면, 엄청난 양의 일을 낭비한 셈입니다.
- 머지하기 쉽습니다. 큰 CL을 작업하는 일은 시간이 많이 소요되기 때문에, 실제로 머지 시에 충돌이 많을 것이고 그러면 더 자주 머지해야 합니다.
- 잘 디자인 하기 쉽습니다. 커다란 수정의 자세한 사항을 전부 개선하는 것보다 디자인과 코드의 상태를 갈고 닦기 훨씬 쉽습니다.
- 리뷰 도중 방해를 덜 받습니다. 전체 수정 중에서 독립적인 일부만 제출해서 리뷰가 진행되는 동안 계속 코딩할 수 있습니다.
- 롤백하기 쉽습니다. 큰 CL은 처음 제출한 CL과 롤백 CL 사이에 업데이트되는 파일을 건드릴 확률이 높아서 롤백을 복잡하게 만듭니다.
리뷰어는 수정이 너무 크다는 이유만으로 당신의 CL을 반려할 재량이 있다는 것을 명심해두십시오. 보통은 당신의 기여에 감사하겠지만, 어떻게든 일련의 작은 수정으로 만들 것을 요청할 것입니다. 수정을 이미 작성한 이후에 작은 수정으로 쪼개는 일은 큰 작업이 되거나, 리뷰어가 왜 당신의 큰 수정을 받아들여야 하는지에 대한 긴 논쟁을 하게 될 수도 있습니다. 처음부터 작은 CL을 작성하는게 훨씬 쉽습니다.
작은 게 뭔가요?
보통 CL의 “옳은” 크기는 하나의 독립적인 수정입니다. 이 뜻은:
- CL이 딱 하나만을 다루는 최소한의 수정입니다. 보통 한번에 전체 기능을 다 하기보다는 그 기능의 딱 한 부분만을 뜻합니다. 대개는 너무 큰 CL에서 실수하는 것보다 아주 작은 CL을 작성하다가 실수하는게 더 낫습니다. 리뷰어랑 적당한 크기가 얼마일지 얘기해보는 것도 좋습니다.
- CL은 관련된 테스트 코드를 포함해야 합니다.
- 리뷰어가 CL을 이해하기 위해 필요한 모든 것이 CL, CL에 대한 설명, 기존 코드베이스, 또는 이미 리뷰한 CL에 있어야 합니다. 나중에 개발해야 하는 사항은 제외합니다.
- CL이 반영되고 난 이후에도 사용자와 개발자를 위해 시스템이 잘 동작해야 합니다.
- 의미를 이해하기 어려울 정도로 CL이 작으면 안됩니다. 새로운 API를 추가한다면, API 사용법도 같은 CL에 포함해서 그 API가 어떻게 사용될지 리뷰어가 이해하기 쉽도록 해야 합니다. 이렇게 해서 사용하지 않는 API가 반영되는 것도 막습니다.
“너무 크다”에 대한 단단하고 빠른 규칙은 없습니다. 100 줄이면 대개 적절한 크기이고, 1,000줄이면 보통 너무 크다고 판단하지만, 리뷰어의 판단에 달려있습니다. 수정이 몇 개의 파일을 건드리는지도 역시 “크기”에 영향을 줍니다. 한 파일의 200 줄을 수정하는 것은 괜찮지만, 200 줄을 50개 파일어 걸쳐 수정하는 것은 보통 너무 큽니다.
당신은 코드를 작성한 그 순간부터 그 코드가 친밀하게 느껴질테지만, 리뷰어는 그러한 맥락이 없다는 것을 잊지 말아야 합니다. 당신에게는 적적한 크기의 CL이 리뷰어에게는 압도적인 크기일 수도 있습니다. 헷갈리면 더 작은 CL을 작성해봅시다. 리뷰어는 CL이 너무 작다고 불평하지 않을 것입니다.
When are Large CLs Okay?
There are a few situations in which large changes aren’t as bad:
- You can usually count deletion of an entire file as being just one line of change, because it doesn’t take the reviewer very long to review.
- Sometimes a large CL has been generated by an automatic refactoring tool that you trust completely, and the reviewer’s job is just to verify and say that they really do want the change. These CLs can be larger, although some of the caveats from above (such as merging and testing) still apply.
Splitting by Files
Another way to split up a CL is by groupings of files that will require different reviewers but are otherwise self-contained changes.
For example: you send off one CL for modifications to a protocol buffer and another CL for changes to the code that uses that proto. You have to submit the proto CL before the code CL, but they can both be reviewed simultaneously. If you do this, you might want to inform both sets of reviewers about the other CL that you wrote, so that they have context for your changes.
Another example: you send one CL for a code change and another for the configuration or experiment that uses that code; this is easier to roll back too, if necessary, as configuration/experiment files are sometimes pushed to production faster than code changes.
Separate Out Refactorings
It’s usually best to do refactorings in a separate CL from feature changes or bug fixes. For example, moving and renaming a class should be in a different CL from fixing a bug in that class. It is much easier for reviewers to understand the changes introduced by each CL when they are separate.
Small cleanups such as fixing a local variable name can be included inside of a feature change or bug fix CL, though. It’s up to the judgment of developers and reviewers to decide when a refactoring is so large that it will make the review more difficult if included in your current 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).
Don’t Break the Build
If you have several CLs that depend on each other, you need to find a way to make sure the whole system keeps working after each CL is submitted. Otherwise you might break the build for all your fellow developers for a few minutes between your CL submissions (or even longer if something goes wrong unexpectedly with your later CL submissions).
Can’t Make it Small Enough
Sometimes you will encounter situations where it seems like your CL has to be large. This is very rarely true. Authors who practice writing small CLs can almost always find a way to decompose functionality into a series of small changes.
Before writing a large CL, consider whether preceding it with a refactoring-only CL could pave the way for a cleaner implementation. Talk to your teammates and see if anybody has thoughts on how to implement the functionality in small CLs instead.
If all of these options fail (which should be extremely rare) then get consent from your reviewers in advance to review a large CL, so they are warned about what is coming. In this situation, expect to be going through the review process for a long time, be vigilant about not introducing bugs, and be extra diligent about writing tests.