Approver is also a Committer

たまに「良さそうに見えます。LGTM！！」というコメントを見ることがあるけれど、そもそもコードレビューっていつ Approve 出すべきなんだろうという小さな悩みが出てきたので考えてみた。

Google の eng-practices には、以下のような記述がある。

In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect.

「レビュアーは CL が確実にコード全体の健康状態を向上させる状態になったのであれば Approve しましょう」。ふむふむ。
「確実に健康状態を向上させる」を判断するには、何を判断すればいいのだろう。
同ガイドライン内の What to look for in a code review も見てみる。
Summary には以下のように書いてある。

In doing a code review, you should make sure that:

The code is well-designed.
The functionality is good for the users of the code.
Any UI changes are sensible and look good.
Any parallel programming is done safely.
The code isn’t more complex than it needs to be.
The developer isn’t implementing things they might need in the future but don’t know they need now.
Code has appropriate unit tests.
Tests are well-designed.
The developer used clear names for everything.
Comments are clear and useful, and mostly explain why instead of what.
Code is appropriately documented (generally in g3doc).
The code conforms to our style guides.

Make sure to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.

Make sure to review every line of code you’ve been asked to review とかを見る限り、全ての行を理解できている必要がありそうだ。
しかも他の項目を見ても設計意図などの深い部分まで見ている。

というあたりまで調べて自分の中での結論は、一言でいうと Approver is also a Committer かなとなった。
その心としては、レビュイーが作ったコード形式、選んだ設計の1つを尊重しつつもレビュアーは自分がそれを書いたくらいの立場になってレビューを行って、自分が書いてもこれが最高のコードだ！となったら Approve するという感じなのかもしれない。
というわけで「良さそうに見えます。LGTM！！」はこういうことができていない兆候なのかもしれない。
またコードレビューは非同期的ペアプログラミングに近いような気もしてきた。