コードレビューの価値観
レビュアーの価値観
コードレビューはすべてのマージリクエストで必須です。私たちのコードレビューの価値観に慣れ親しみ、それに従うべきです。
レビュアーのミッション
レビュアーとして、私たちは GitLab コードベースの保守性を低下させることなく、フレンドリーで尊重し合える環境の中で愛されるフィーチャーを構築しながら、開発速度を高めることを目指します。
ステークホルダーの優先事項
マージリクエストのすべてのステークホルダーの目標と目的を重視していますが、ステークホルダーの価値観が競合する場合は、以下の優先順位で考えます:
ユーザー > コントリビューター > 自分自身
レビュアー/メンテナーは要件を所有していないことを念頭に置くことが重要です。要件を作成・維持するのは最終的にプロダクト/UX の仕事です。 コントリビューションで導入される機能について懸念がある場合は、PM を関与させるようにしてください。 ただし、私たち全員がエンドユーザー体験全体に目を向ける役割を担っています。
歴史
GitLab には、マージリクエストのレビューで最高品質を目指す多数のレビュアーがいます。しかし人間である以上、個人的なニュアンスの影響を受けます。
これは @sarahghp によって Frontend メンテナーミーティングで議論ポイントとして提起されました。*「チーム間のコードの一貫性の理想的なバランスとは何でしょうか？一貫性を追求するためにあまりにも細かいことを指摘しすぎていませんか？」*という問いが、より大きな問いへと発展しました:
レビュアーとして、レビューで強制すべき価値観は何で、より柔軟に対応できるものは何でしょうか？より柔軟に対応できるアイテムについて、どのようにアプローチすべきでしょうか？レビュアーが作者とブランチを共有する協力モデルを目指すべきか、フォローアップ Issue とマージリクエストを促すべきか？それとも個人の好みとして考えられるものでしょうか？
簡単にまとめると:
マージリクエストが私たちの価値観に反する場合、それをブロックすることを許可すべきでしょうか？
これらの問いを踏まえ、このページに特定の価値観のセットを文書化し、会社全体で一貫したレビューを実施できるようにしました。
TL;DR
- マージの全体的なコード品質に実質的な影響のない小さな問題に行き詰まらないようにします。
- 技術的負債の導入を避け、その削減に努めます。
- シンプルでつまらない解決策を奨励しますが、要求はしません。
- 同僚のレビュアーに頻繁に相談しますが、特に関係者間に大きなタイムゾーンの差がある場合は、作者の遅延に配慮します。
- 変更の広範な影響と、コードが全体的なコードベースをより良くするか悪くするかを考慮します。すべてはトレードオフです。
- 依存関係を導入する際は慎重になり、その副作用を考慮します。
- わからないことは認めます。
- 親切、フレンドリー、謙虚であること。
- 要求を避け、代わりにコーチングと教育をします。
- 行動と協力へのバイアスを傾けます。
- 人々が安全に失敗できるスペースを作り、自分自身も安全な過ちを犯し、革新的なアプローチを試みます。創造性を歓迎します。
マージ収益性
一般的に、マージリクエストが純利益であれば、積極的に承認して前進すべきです。この考え方は以下のようなことを考慮に入れています:
- 品質の必須事項（permission-to-play とも呼ばれる）
- ユーザーにとっての変更の価値
- 将来の保守コストの価値
- 機会コスト
すべてのメリットとデメリットの合計が純利益となり、前進できるようにする必要があります。
解決策はない、あるのはトレードオフだけだ。（Thomas Sowell）
ソフトウェアには完全な解決策はありません。それらは存在しません。 これは主にコンピューターの性質によるものです（例えば、CAP 定理を参照）。 また、品質属性はしばしば互いに相反します。例えば、セキュリティの強化はパフォーマンスを損ない、一部のパフォーマンス向上は保守できません。ソフトウェアエンジニアリングは優先順位付けとバランスに関するものです。
優れたレビュアーになるための最初かつ最も重要なステップは、コードベース内で許容できるトレードオフを受け入れ、管理し、伝えることを学ぶことです。
品質の必須事項
マージリクエストが「収益性がある」と見なされるためには、いくつかのものが必要です。 レビュアーであることは難しいバランス作業です。イテレーションと品質の懸念のバランスを取ることがしばしばありますが、品質へのバイアスは自分自身（とチームメイト）に役立ちます！以下は、一般的な（ただし網羅的ではない）品質の必須事項のガイドです:
- ユーザー体験。変更は有用で堅牢な、愛されるユーザー体験を生み出していますか？
- 信頼性。コードは信頼できますか？コードは毎回同じ出力を返していますか、それとも多くの予測不可能な副作用を導入していますか？
- 依存関係と脆弱性。このマージリクエストのコードはコードベースに脆弱性、または外部・内部の依存関係を導入していますか？依存関係を使うたびに、それはサポートが必要な別の何かになることを念頭に置いてください。
- 保守性。関連するコードへの変更（例えば、バグ修正や要件変更）を簡単に行えますか？必要以上のコードを追加していませんか？
- 可読性。新しいコントリビューターが関連するコードに飛び込んで何が起きているかを理解できますか？
- テスト。変更に対してテストはほぼ常に提供されるべきです。テストは機能のユースケースとコンポーネントコントラクトをカバーしていますか？テストは将来の変更でユーザー体験が愛されるものであり続けることを保証しますか？
- 一貫性。変更セットは高いレベルと低いレベルの両方で既存のコードと一貫していますか？
これらの必須事項は、プレイへの許可を確立する際のすべてのコードレビューの出発点となるべきです。各レビュアーには独自のレビュースタイルがあり、それが個人のレビュアーとしての特徴です。それぞれの専門知識を提供できますが、一般的に、GitLab での高品質な基準とステークホルダーへのサービスを集団的に確保することを目指しています。
すべての変更が同等に作られているわけではないことを念頭に置いてください。作者がレガシーコードに取り組んでいる場合、グリーンフィールドコードと同じくらい高い品質要件を持つべきではありません。変更はそれでもコード品質の純利益につながるべきです。
純損失から純利益への協力
マージリクエストをレビューして品質のトレードオフを特定した場合、うまく伝えることを目指してください:
- 見たトレードオフを説明します。
- なぜそれがトレードオフだと思うかを説明します。
- マージ作者と共に解決策に向けて取り組むことを目指します。
これにより、行動と協力へのバイアスが生まれます。作者はレビュアーにとって難しい選択を導入したことに気づいていないかもしれないので、コミュニケーションには親切さと謙虚さをもって取り組んでください。作者は単に問題を解決するために良いコードを書こうとしただけかもしれません。作者はしばしば自分の選択の広範な影響を予見しません（そうする必要もありません。それは進行の欠如につながります）。
速度
GitLab では、継続的にイテレーションを行い、これをマージリクエストの妥当性判断の核心的な価値観と考えています。GitLab でイテレーションを重視するのは、問題をできる限り小さな部分に分解することと、行動へのバイアスを求めるためです。
これはレビュアーの仕事を楽にする指導原則として本当に信じています。コンテンツが小さく消化しやすいためです。ただし、複数のイテレーションが常に物事を簡単にするわけではないことも認識することが重要です。時にはコンテキストを把握するのが困難になることがあります。しかし、これらの問題は高速度、フィードバック、信頼性という利点によって上回られます。これはレビュアーとして、作者に常に奨励すべきことです。「これをもっと小さくできますか？」は、レビューを始める際に自問すべき非常に一般的な問いです。
小さな自信を持ったステップをたくさん踏んで、混乱に至ることは十分にあり得る。（Paul Slaughter）
イテレーション的アプローチはまた、大きなまたは複雑な問題の解決策を、小さく品質重視の部分に分解することを確認することで考え抜くよう作者を教育します。
レビュアーとして注意すべき品質の実際的な（ただし網羅的ではない）例:
- コードは読みやすく、簡単に理解できますか？
- 一度に1つの Issue を解決していますか？
- 壊れやすいですか？
- 簡単に拡張または変更できますか？
- コンポーネントの入出力を正しく表現するサポートスペックがありますか？
- システムの単位は疎結合されていて、一方が隠れた、または脆弱な方法で他方に依存していませんか？
- システムの単位は高凝集されていて、個々の単位が単一の責任と変更の1つの理由を持っていますか？
誠実さ
誠実であることは難しいことです。マージ作者とマージレビュアーの間に権威の不均衡があると認識される場合はさらに難しくなります:
- 作者として、難しいまたはブロッキングな質問をされたときにコードを通過させるために誤解を招く可能性があります。
- レビュアーとして、何かを理解していないことを認めることをスキップしたくなることがあります。
しかし、これはあってはならないことです。ここでやりたいのは、感じられる権威バイアスを減らすことで、作者とレビュアーの間に恐れのないコラボレーション感覚を常に奨励することです。
レビュアーとして、レビューにおいて何よりも誠実さを保つ必要があります。何かを理解していない場合は、作者または別のレビュアーに尋ねてください。作者が取ったアプローチに同意しない場合も同じです。これを声に出すことは問題ありませんが、レビュアーとして、あなたのアプローチへの不同意がアプローチの根本的な欠陥であることを確認する責任があり、それが単なる個人的な好みではないことを確認します。
プロヒント: 疑わしい場合は、別のレビュアーに健全性チェックを求めてください。
最悪のケース分析
レビューしているマージリクエストに未知のものがあると気づいた状況では、絶対に最悪のケースのシナリオが何かを考えてみてください。最悪のケースでもこのマージリクエストが純利益のままであれば（かつ permission-to-play の問題がなければ）、知識を共有し協力するためにノンブロッキングなコメントを残すことを検討してください。
スペースを作る
GitLab では、できる限りインクルーシブであることを目指しており、これには人々が自分のやり方で成長し学ぶためのスペースを作ることも含まれます。レビュアーとして、作者が質問をし、過ちを犯し、素晴らしいものを作ろうとする試みで恐れを知らない気持ちを奨励してください。GitLab での最高品質のコードを望む一方で、作者がレビューを求めてサブミットボタンを押す際に必要な安全ネットを作る思いやりのスペースも必要です。
品質ベンチマークを満たさないマージリクエストに取り組んでいる場合は、作者がそれを学習体験として吸収できるようにしてください。また、再挑戦してより良いマージリクエストを作ろうとインスパイアされる正しい反応を育み、何か不足しているとみなされることへの恐れではない反応を育んでください。
作者が学習体験を吸収する際、レビュアーとしても学習体験として受け入れることを許してください。遭遇する各問題は、作者とチームの全員が学ぶ機会です。
ニットピック
完璧主義は進歩の敵だ。（Winston Churchill）
ニットピックとは、個人的な好みや些細なコード改善に対する軽微な変更のリクエストです。コードレビューで多くのノイズを生み出し、最良の場合はレビューを停滞させ、最悪の場合は作者にとって有害な体験を生み出します。
フィードバックを書く際は、conventional comments を使用して意図を明確に伝えてください。特に、blocking と non-blocking のデコレーターを活用して優先度を示してください:
blocking: この問題が解決されるまでマージリクエストをマージすべきではありません。non-blocking: 作者がスキップできる提案または観察。MR が主にディスカッションや探索のためでない限り、non-blocking なアイテムは必ずしも対処されないことが了解されています。
コメントを「ニットピック」と表現することを避けてください。コメントがマージリクエストをブロックするほどの価値がない場合は、代わりに non-blocking とマークしてください。これにより、追加のレビューサイクルという暗黙のプレッシャーを取り除きながらフィードバックを保持します。
レビュアーとして、すべてのフィードバックが non-blocking であることに気づいた場合は、追加のレビューサイクルを要求せずに承認（およびマージ）することを検討してください。
このトピックの詳細については、ニットピックをやめるのブログ投稿をご覧ください。
