Google's Code Review Guidelines を読んでアクションに落とす
Summary
Google's Code Review Guidelines を読み、具体的にどうアクションするかを念頭に置くため、アクションリストだけをまとめました。
- Google Engineering Practices Documentation | eng-practices
- 2019/9/6頃に公開されました。
- Google エンジニアリング・プラクティス ドキュメント | eng-practices
- 2019/9/16頃には全ての翻訳が完了しています。
- 非公式の日本語翻訳。今回まとめる際に大変参考になりました。
レビュアー向けのガイド
The Standard of Code Review 基準
- プルリクのコードが完璧でなくても、確実に改善するのであれば、積極的に承認する。
- 重要でない指摘は、”Nit:” をつけることで、変更するか否かは作者に委ねることを伝える。
- コードレビュー上のコメントでのやりとりが難航しそうなとき、対面のミーティングやビデオ会議を行う。
- そのとき、他のレビューワーのために、議論の結果はコメントとして記録する。
- 合意を得られないからといって、プルリクを放置しない。
What to Look For In a Code Review 何を見るか
- ユーザーへの影響を理解するにはコードだけでは分からないこともあるので、動作確認を行う。
- 難しい場合は、作者にデモをお願いする。
- オーバーエンジニアリングに警戒する。
- 適切なテストを求める。
- コード中のコメントがWhyであるか、Whatになっていないかを確認する。コメントが必要なものかを確認する。
- 個人的なスタイルの好みに基づくものは“Nit:”とする。
- 関連するドキュメントのアップデートを依頼する。
- すべての行を読み、自分で理解する。
- コードを読むのが難しい場合は、明快にするよう作者に伝える。
- レビュアーとして自分がふさわしくないと感じるのなら、他の人をレビュアーにアサインする。
- コードの変更で良いところがあったら、それを作者に伝える。
Navigating a CL in Review どう進めるか
- ステップ1: 説明をよく読む
- プルリクの説明はよく読む。
- 変更が行われるべきでない場合は、すみやかに、理由・代替案を丁寧に説明する。
- 望ましくない変更が多く発生する場合は、コードレビューの前の開発プロセスを見直す。
- ステップ2: メインとなる部分を見つけ、設計を確認する
- まず、メインとなる部分を探す。
- コードの変更が大きい場合は、分割を依頼する。
- 設計に問題がある場合は、すみやかにコメントを送る。
- ステップ3: 他の部分を読む
Speed of Code Reviews スピード
- 集中しているときでなければ、レビューがきたらすぐに行う。レビューで自分の集中を妨げない。
- コーディングが完了したとき、昼食の後、キッチンから戻ってきたときにレビューを行う。
- 1営業日中までにレスポンスする。
- レビューは翌朝の最初に行う。
- 忙しい時は、いつできるかをレスポンスする。他のレビュアーを提案する。最初に大雑把なコメントをする。
- 未解決なコメントがあったとしても、細かい変更であったり、後で変更されるだろうと言う場合は、承認する。
How to Write Code Review Comments コメントの書き方
- 礼儀、あいまいではなくはっきりとした態度、尊敬する気持ちをもつ
- 開発者に対してコメントをしない。コードについてコメントをする。
- 「なぜか」を説明する。
- 問題を指摘することと、具体的なコードを提供することの間で適切なバランスを取る
Handling Pushback in Code Reviews
- 対立がおきたとき:
- 立ち止まり、開発者の意見が正しいのではと考える。
- 理解しようとする姿勢が伝わるようにする。
- なぜ、提案が正しいと思うのかを、丁寧に説明する。
- 後でやる、が発生したとき:
- 「後でやる」が実行されることは少ないので、今すぐやるべきと主張する
- すぐできないときは、チケットを切る
- レビュー基準のブレにより不満が発生したとき:
- 恐らくその不満は、レビューのスピードをあげることで解消できるので、それを検討する
プルリクの作成者向け
Writing Good CL Descriptions
- 1行目:短い要約。コードの変更によって具体的に何がなされたのか。(英語の場合は、命令形を使う)
- 残りの本文は、十分な説明を書く。何を解決したか。なぜそれが良い解決方法であるのか。欠点は何か。なぜ変更したか。
- プルリクを送信する前に、見直す。
Small CLs
- コードの変更は、1つのことだけを扱う。
- 100行は妥当、1000行は大きすぎる。1ファイルの200行は問題ないかもしれないが、50ファイルにわたる200行は大きすぎる。
- 分割した場合は、他のプルリクがあることを伝える。
- リファクタリングは分離する。
- 関連するテストコードは分離しない。
- 大きなコード変更をする前に、先にリファクタリングのみ行う。
- コードの変更が大きくなった時は、レビュアーに了承を得る。
How to Handle Reviewer Comments
- コメントに怒りで返事をしてはいけない。
- レビュアーのコメントが建設的でないときは、対面かビデオ会議する。
- レビュアーがコードを理解できなかった場合、コードを適切に変更するか、コードにコメントを残す。
- コメントが来たとき、反射的に考えるのではなく、立ち止まり、そのコメントは正しいか?をよく考える。
- 対立したら:
- コードレビューの基準を読む。
SET and REMOVE in an UpdateExpression DynamoDB
DynamoDB の put-item にて、一部の属性の値のみを更新しつつ、とある属性を削除したかったため、動作を確認しました。
An update expression consists of one or more clauses. Each clause begins with a SET, REMOVE, ADD, or DELETE keyword. You can include any of these clauses in an update expression, in any order. However, each action keyword can appear only once.
ドキュメントにもある通り、put-item では、SET, REMOVE, ADD, DELETE のアクションを指定でき、それらを複数指定できます。これらのアクションは、put-item内で1度のみしか 使えないようです。
事前準備
テーブル TableName: Members, Key: id
を作成します。
aws dynamodb create-table \ --table-name Members \ --attribute-definitions \ AttributeName=id,AttributeType=S \ --key-schema AttributeName=id,KeyType=HASH \ --billing-mode PAY_PER_REQUEST
データを2件投入します。
id: "1001", name: "takada", group_id: "100", location: "Tokyo"
id: "1002", name: "ishikawa", group_id: "200", location: "Hokkaido"
aws dynamodb put-item \ --table-name Members \ --item \ "{\"id\": {\"S\": \"1001\"}, \"name\": {\"S\": \"takada\"}, \"group_id\": {\"S\": \"100\"}, \"location\": {\"S\": \"Tokyo\"}}"
aws dynamodb put-item \ --table-name Members \ --item \ "{\"id\": {\"S\": \"1002\"}, \"name\": {\"S\": \"ishikawa\"}, \"group_id\": {\"S\": \"200\"}, \"location\": {\"S\": \"Hokkaido\"}}"
SET のみ
まずは SET のみを使い、一部の属性のみ更新できるかを確認します。
id:1001 の location を、Tokyo から Chiba に変更します。location は、DynamoDB の予約語なので、--expression-attribute-names
で#location
を定義します。また、--return-values ALL_NEW
で更新後の結果を返すように指定します。
結果を見ると、location 以外の値は変更されていないことを確認できます。
aws dynamodb update-item \ --table-name Members \ --key "{ \"id\": { \"S\":\"1001\" } }" \ --expression-attribute-names "{ \"#location\": \"location\" }" \ --update-expression "SET #location = :location_value" \ --expression-attribute-values \ " { \":location_value\": { \"S\": \"Chiba\" } }" \ --return-values ALL_NEW
{ "Attributes": { "location": { "S": "Chiba" }, "id": { "S": "1001" }, "name": { "S": "takada" }, "group_id": { "S": "100" } } }
SET と REMOVE
location
を変更しつつ、group_id
を削除する put-item のオペレーションを実行してみます。ちなみに、group_id
が削除されている状態のときに実行しても問題なく成功します。
update-expression はこのように指定します: --update-expression "SET #location = :location_value REMOVE group_id"
。REMOVE
の前に,
は不要です。
aws dynamodb update-item \ --table-name Members \ --key "{ \"id\": { \"S\":\"1001\" } }" \ --expression-attribute-names "{ \"#location\": \"location\" }" \ --update-expression "SET #location = :location_value REMOVE group_id" \ --expression-attribute-values \ " { \":location_value\": { \"S\": \"Kyoto\" } }" \ --return-values ALL_NEW
{ "Attributes": { "id": { "S": "1001" }, "name": { "S": "takada" }, "location": { "S": "Kyoto" } } }
お試し
ここでお試しとして、同じ属性を SET と REMOVE で指定してみます。このように: --update-expression "SET #location = :location_value REMOVE #location"
。実際に使うことはないと思われます。
結果は、以下のようにエラーが発生します。各アクションで同じ属性(document paths)は指定できないということですね。
aws dynamodb update-item \ --table-name Members \ --key "{ \"id\": { \"S\":\"1001\" } }" \ --expression-attribute-names "{ \"#location\": \"location\" }" \ --update-expression "SET #location = :location_value REMOVE #location" \ --expression-attribute-values \ " { \":location_value\": { \"S\": \"Kagawa\" } }" \ --return-values ALL_NEW
An error occurred (ValidationException) when calling the UpdateItem operation: Invalid UpdateExpression: Two document paths overlap with each other; must remove or rewrite one of these paths; path one: [location], path two: [location]
さらにお試しとして、SET -> REMOVE -> SET を指定してみます。このように: --update-expression "SET #location = :location_value REMOVE group_id SET #name = :name_value"。ちなみにname
も DynamoDB の予約語です。こちらも実際に使うことはないと思われます。
SET が複数指定されているので、ドキュメントにも記載されているようにエラーが発生します。
aws dynamodb update-item \ --table-name Members \ --key "{ \"id\": { \"S\":\"1001\" } }" \ --expression-attribute-names "{ \"#location\": \"location\", \"#name\": \"name\" }" \ --update-expression "SET #location = :location_value REMOVE group_id SET #name = :name_value" \ --expression-attribute-values \ " { \":location_value\": { \"S\": \"Kagawa\" }, \":name_value\": { \"S\": \"Doraemon\" } }" \ --return-values ALL_NEW
An error occurred (ValidationException) when calling the UpdateItem operation: Invalid UpdateExpression: The "SET" section can only be used once in an update expression;