日々常々

ふつうのプログラマがあたりまえにしたいこと。

コードレビューのコメントすべてに対応する必要があるか

いろんな現場でちょいちょい話題になるので「前にツイートしたなぁ」と見てみたらすっごい長かった。

諸悪の根源?

とりあえず全文転記。誤変換とかもそのまま。

コメントあると「対応しなければならない」ってなるの。あれが諸悪の根源だと思ってる。

目に見えてる問題を潰したくなるのはごく当たり前の感情だと思うけど、目に見えてるもの全て潰したからと言って完璧にはならないことは理解する必要がある。言えるのは「目に見えるものはない」というだけで……そしてコメント対応とかだと、「見た目」のメタファすら使えなくなる。

「コメント対応」のような問題の潰し方をすると必ず歪になる。これは自分の目で見つけたものなら曲がりなりにも「自分の目」の精度は高くないものの一定の見方ではあるのに対し、コメントは特定部分になるので、一貫性のない歪になるのが確約される。その人には見えないように、はできるかもだけど。

ともかく「コメントは全て対応しなければならない」と言うのは、そのほうがいいって刷り込みだったり、対応しなければならないって強迫観念だったりで、ゼロリスク進行にも通じるものがあると思う。こう言うのがあると「コメントしたら進まなくなるし今回はやめておこう」とか変な力が働いたりする。

issueが数十件ある某リポジトリに「こんないっぱい問題あるんですね」とか言われたのだけど、ゼロにするの目標にしたりしてないし、制御できてるならそれでいいと思うんだよね。数千とかなって見る気も対応する気も失せるのとは違うと思うのだけど、ゼロありきの思考だと同じに見えてしまう。

コメントはコメント、対応するかどうかは別の問題……と言っても、スルーだとこれはこれでコメントする気が失せるので、何かしらのリアクションと言うか、変化は必要。何も変化させない行為は無駄と感じてしなくなるものだと思う。実際無駄なわけだし。

と言うことで「コメントを対応しなければならない」と考えるのは、対応し切るまでそこで停滞するし、それを嫌うとコメントそのものをしづらくなるし、コメントされた側は凹んだりするし(面倒なのはわかる)、対応したところで歪にしかならない。ほんと悪いところしかない。まず対応の確約を捨てるべき。

少し語弊があるな。対応に「コメント見ました」で特に対象物を変更しない、と言うのも含めるなら対応するとしても良い。とにかくコメントに対して「何か変更を加えてコメントが一切出なくするのが理想」みたいなのを信仰してたりする。はっきり言って呪いだと思う。

どうやったらしなくて良いようにできるかで、「別課題で管理します」みたいなのがある。これはそれなりに有効なのだけど、その課題を作る手間もかかるし、同様にして出てきた他の課題との重複とかも確認しなきゃ、そうして作られた課題群はただのゴミの山になる。何もないよりマシかどうかは微妙。

レビューとかのコメントは有効なんだ。有効なんだけど、効くところが限定的。でもそれでしか拾えないものがあって、外せないものになる。外せないからここで賄えそうなものは全部寄せられる。結果、得意分野じゃないとこも合わせた定量評価をしようと言う動きが出てくる。どれも順当なのに結果は不幸。

人がコメントすることでしか賄えないものと、人が作るべきものってのは多分一致する。機械的に検出できるもの、そして機械的に対応できるものは作らなくて良い。開発の道具はそうして進歩してきたわけで、その先のいくつかの候補がたまにバズるアレとかソレなんだろう。その活動全部でプログラミング。

コメントは「ここから見たらこう見える」と言う、見えたことを語るものなんじゃないかなってのが最近思ってることで。そこに無理矢理複数の視点を混ぜるから、玉石混淆になってしまい、扱いに困った結果「全部対応」の引力に飲み込まれる。とは言え人がやる以上「ふと気づいた」は避けれなかったりも。

コメントの全対応を強いるのは、コメントと対応を定量評価してるからで、そうさせたのはコメントに様々な期待を相乗りせたこと。そもそもが人間の感覚に頼ったピーキーなものなのに、そこで見つけられたものに対処するのは一定の成果を挙げてきたから強化されてきたが、このモデルが軋みを上げている。

きっと計算機が高額で遅かった頃は合理的だったんだ。そして多分とっくにひっくり返ってる。なのでこの一連のスレッドのコンテキストを理解できない組織にいる人も居るんだろうけど、なんでもコメントして全部対応に寄りかかってるとこはまだまだ多い印象ある。

「全てのコメントを片付けなければならないと考えるのが諸悪の根源」として始めたのに、全然根源でない……と言うか、表出した事象に過ぎなかった。表出した事象は短絡的に潰してはいけない。掘り下げて出てきたものも、まだ表層かもしれない。根っこはどこだ……

機械的に対応できるような、人がしなくてもいいコメントが含まれてるのは一因。これは検証済みマージとかと同様に機械的なチェックでカバーして棲み分ければ減らせるかもしれない。この性質のコメントの割合が減れば、対応の難度が上がり、全対応の無理筋加減はわかりやすくなるかもしれない。

これはコメントのほとんどが機械的に対応できるものなので、コメントはそう言うものと認識されていると言う仮説。この手のコメントは誤りの指摘であり、多いのは事実。このことが「コメントされたら直すもの」「コメントされるとは誤っている」と考えてしまうことに繋がる。のかもしれない。

https://x.com/irof/status/1306446734176862208

いや長いな。ふむふむ、と読んでたんだけど最後のほうで

「全てのコメントを片付けなければならないと考えるのが諸悪の根源」として始めたのに、全然根源でない……と言うか、表出した事象に過ぎなかった。

とか言ってて「おいっ」ってなった。まぁいいや。とりあえずこれをChatGPTに要約してもらう。

コメントに全て対応しなければならないという考え方が問題だと述べています。目に見える問題を全て解決しても、完璧にはならないことを理解する必要があります。コメント対応に追われると、一貫性のない対応になりがちです。全てのコメントに対応することが理想と考えるのは誤りで、時には「見ました」と言うだけの対応でも十分です。コメント対応は合理的に管理し、必要ならば機械的に処理できる部分を分けて対応の負担を減らすことが重要です。コメントは多くの場合、人間の感覚に基づいているため、全対応を目指すのは非現実的です。

そんなこと書いたっけ……?あってるような、ちょっと違うような。

「挙げている問題点」を抜き出してもらうとこんな感じ。

  • 一貫性のない対応:
    • コメントに全対応すると、特定部分に偏った対応になり、一貫性のない歪な結果になる。
  • 進行の停滞:
    • コメントに全対応しようとするとプロジェクトの進行が停滞し、コメントする意欲も削がれる。
  • 対応の過剰負担:
    • 機械的に対応できるコメントが多く含まれており、これに全対応しようとするのは非現実的で負担が大きい。

へぇ。他には?とか聞いてるとどんどん出てくるけど、無理に挙げてる感じで重複もでてくる。

  • ゼロリスク進行の弊害:
    • コメントを全て解決しようとするゼロリスク進行の考え方が、実際のリスク管理や問題解決を歪める。
  • 心理的な負担:
    • コメント対応が多いと、開発者は心理的に圧迫され、モチベーションが下がる可能性がある。
  • 無駄な労力の増加:
    • 必要のない対応に時間や労力を割くことで、重要な作業に集中できなくなる。
  • コメントの質の低下:
    • 全対応を求める風潮があると、コメントが雑になり、質の低いフィードバックが増える可能性がある。
  • コメント対応の遅延:
    • 全てのコメントに対応しようとすると、対応が遅くなり、重要なフィードバックが埋もれてしまう。

あ、「ゼロリスク進行」は「ゼロリスク信仰」の誤変換なんだけど、そのまま謎ワードで解釈されてるね。興味深。

こういうのはすぐ「コメント対応するなってこと?」みたいな極論に振るんで、それも聞いてみる。

要はバランス(キリッ

このスレッドは特に大きくアップデートもないかな。私はこんな感じに考えてる。

コードレビューのコメントに関するスレッドは他にもあって。

コメントしづらくなる問題とレビュアーのできる対応

プルリクエストにコメントすると停滞してしまうのを見ると、「プルリクエスト単体で完成度あげるよりマージして全体の中で整合とった方がいい」派の私は、プルリクエストのコメントを控えるようになってしまう。

控えちゃいけない、自分の職責を果たせ。と思って、なんとかコメントを書き、かつ停滞を避けるために「このプルリクエストで対応する必要はありませんが」などの枕詞を置くが、FYI程度に読み流されたり、マージ後は喉元すぎた感じで忘れられたりすると、この形のコメントはダメなんだなぁと。

おそらくやるべきはPRにコメントじゃなく新規Issueをたてる、なんだろなぁとは思うが、現行プロセスと喧嘩しがち。

どこに書くか迷ってどこにも書かず内心に抱えるくらいならどこかにアウトプットすべき、ではあるんだが、プルリクエストのコメントがあってそうで、なんかあってないんだよなぁ。

むろんこのスレの2つ目のコメントのように「対応不要」と言いつつコメントしたり、approveは出しつつコメント書いたりとかはするんだけど、どうも「コメントは全て対応しなきゃいけない」みたいな謎フォースもある。こういうのとか

コメントはコメントであってコメントでしかないんだよってコメントは結構ある。コメントの重さ的なの示せる機能あったら使うけど、そうでないのは(参考)(無視可)(感想)とかつけたりして。でもなんかスルーになれると対応して欲しいのもスルーされるようになって、まぁなんというか。 https://x.com/irof/status/1458012694741618689

で冒頭の「プルリク単体での完成度より全体整合」を優先してチーム状況が許した場合に選択肢に挙がるのがこれ。触ってるプロダクトのコード全部を共同所有できてる場合には最適なんだけど、そうじゃなかったら弊害も多い。実際うまく行ってるとこといかなかったとこがある。

https://irof.hateblo.jp/entry/2019/10/25/113652

「とっととマージして全体の中で整合取ろうぜ」とか言うと、マージしたあとのブランチ(mainにマージするならmain)が「マージ直後は壊れうる」をヘッジする他の仕組みがいるんだよね。 「壊れないを保証」なんて言い出したら歴史改変につながるから、可能な限り壊れないための施策は施した上で「もし壊れた時は行燈を引いて全員が手を止めて早急に復旧する」になるんだけど、なかなか道は険しくて。一足飛びに行こうとすると事故って戻したくなっちゃう系。

長いスレッドはこの2つくらいかな。他にもちょいちょいあって、いくつか拾ってみる。

他に似たようなので細かいの

「レビュー指摘対応」ってコミットコメントには、どんな指摘だったのかとか、なぜその指摘に従ったのかとか、その辺の情報が足りないのです。 https://x.com/irof/status/752659191299317761

「コメントに対応する」というお仕事になると、コミットコメントには「なぜこの変更をしたのか」の背景をかくべきなので、妥当と言えば妥当なんだよなぁと。

レビューコメントには意図を返して欲しい。……あ、「指摘対応」とかのコメントはそれはそれでそういうメッセージか…… https://x.com/irof/status/956680702296170497

6年前に通った道だった。螺旋かどうかは不明。きっと反復横跳びも大事。

コードレビューを何かのゲートや承認のようなものにしたくないんだよなー。特にPR運用とか、どうしても差分ばっか目立つし……差分以外へのコメントしづらいし……同チームでなにやってんの、って思ったりする…… https://x.com/irof/status/1171755778979422209

コードレビューのコメントは有用だと思うんだけど、ゲートにするかは別の話。PR運用はそれがゲートになりやすく(Approveとか後押しする仕組みもある)、個人的には嫌いで、それが「脱ブランチファースト」のブログにつながってる。タイトルはちょいミスったと思ってるけど、まいいや。

irof.hateblo.jp

先のポストが2019年9月11日でブログが10月25日なので、多分この時期のマイブームだったんだろう。

レビューでコメントしたら「必ず何か変更しなければならない」となってるのはあんまよくなくて、(レビューの位置付けにもよるけど)そう言うもんじゃ無いよーとかは重ねて言わなきゃなんない。

「レビューの位置付けにもよる」とか奥歯に物が挟まったようなのを入れてるのは、レビューがフェーズゲート的な承認プロセスになっている場合とかもあるので。てか何も言わなきゃそっちのが多い気もする。この位置付けのレビューでは全コメントの決着が必要になってきたり、こなかったり。 https://x.com/irof/status/1577809260859121664

ふむ。

コメントするかどうかと、その後どうするかは分けて欲しいんよね。コメントする時にその辺とか考えると言うの控えるようになる。もちろんレビューの目的に沿わないコメントは言わないのがよいだろけど。

指摘された「から」直す、じゃねーんですよ。と。 https://x.com/irof/status/1677624849529974785

この辺りは後で紹介する「間違いだらけの設計レビュー」で「問題検出」「問題指摘」としてわけられてるところ。フェーズを示すほうが振る舞いやすい気はしないでもない。

「静的解析ツールに指摘されて提案されたまま直す」の、序盤はやっていいけれど、ある程度理解したらやってはいけない系になるものも多いです。コミットコメントに「静的解析の指摘対応」とか出てくるのはそのスメル。 https://x.com/irof/status/1651145378291662848

コミットコメントでありがちでイヤーな感じのする筆頭がこの手の「ツールに指摘されて直せっていわれた通り直してます!」と言うもの。昔は「こんなコメントはダメだ!」みたいなこと言ってたんですが、ここでは「スメル」にトーンダウン。というのも「ダメ」って言うと誤魔化しコメントを書くようになるだけなんですよね。なので「指摘対応」としかできないなら「指摘対応」としてその事実を記録しとくでいいや、と言う感じ。表出した事象を潰してはいけない。

参考

最近第3版が出て、アジャイルやAIの話が盛り込まれました。「設計レビュー」と題されていますが、ソフトウェア開発における大体のレビューに適用できます。

こちらの本、というかレビューの本はだいたい「問題」にフォーカスしているため、コードレビューを通じた技術知識の伝達などにはあまり触れられていません。どちらかというと本書で言う「二兎追い」や「知識のひけらかし」と言った間違い系として扱われます。PRレビューなどをゲートとして扱うのであれば合ってる。

レビュー技法も大まかに触れられていて、大分類に「マネジメントレビュー」「オーディット」「ピアレビュー」、このピアレビューの小分類で「ウォークスルー」「テクニカルレビュー」「インスペクション」が挙げられています。 この辺りはレビューに向き合うときベースとして持っていると整理しやすい知識です。「レビュー」は特に人間系で行われる活動なので、不定形の化け物になっちゃいがちなので、枠があると考えやすい、はず。

インスペクションが厳格として挙げられていますが、本書の解説だとどれくらい厳格かは分かりづらいと思います。私が読んだことあるのはこの辺。

レビューで厳格さを求めるならこれくらいやらなきゃなんない、ってのがわかって、いい感じに折り合いをつけられるようになるかと。私は実務でインスペクションは試したくらいで、本格的に導入したことはないです。たまにカンフル剤的にやるのはいいかも。

あとこの文脈だとこの辺

デザインってついてますが、グラフィカルなデザインだけの話ではないです。どっちかってと批評、コミュニケーションの本。 おまけのポスターは印刷して部屋の壁に貼ってる。

まーでもわかるんだよ

レビューイ、レビュアーの関係って簡単に上下関係ができちゃうんだよね。指摘される側と指摘する側。 元々上下関係がある場合もあるし、レビューを通してできていっちゃう場合もある。そうしたくなくても瑕疵が指摘されると謎の上下関係ができる。何だそりゃって思うし嫌いなんだけど、でも自分も近い振る舞いをしている気はする。そんなつもりなくても。指摘内容によっては「正しい」「間違ってる」が明確なものもあるから、そういう関係性にするフォースはかなり強い。

レビューしていただく側(いただくとか言う表現をあえて使う)はご指摘を賜ったら意に沿って然るべき……書いててゾワゾワした。

ベテランの人のコメントは本人が軽い気持ちだったとしても、その意図を忖度しちゃうとかはありがちで。なのでコメントする側が気を配らないといけないとは思う。 思うんだけど、その気を配るコストを常にかけるのはそれはそれで費用対効果を考えると微妙にも思える。気を配った結果コメントするコストが爆増して「(すべき)コメントしない」ってなるのも本末転倒だし。

コードはコード、人は人、コードへの指摘は人格攻撃ではない。なんてことを言っても、コードに自分の思いを載せれば載せるほど愛着は湧くわけで。コードの共同所有などをうまく機能するにはそういうの必要だと思うし、愛着があればあるほど文句を言われればイラっとするし、貶められたと感じて、防御反応は出る。防御しちゃったら突破されたときに屈服、服従になる。そうなると勝者敗者の関係になって、全て従うのが正しくなるだろう。そんなこと考えると、そもそもそんな争いをせずに済ませようってなり、「指摘対応」という最適化された行動や無難なコメントになるんだと思う。お互いが様子見してぶつからないように、という行動。タックマンモデルでいう形成期の様子見な振る舞いかしら。成長するならいいんだけど。

レビュアー、レビューイに上下関係はない。綺麗事でもなんでもなく、あっちゃいけないんだ。対等な関係でできるのがいいと思うものの、いきなり「対等でなければできないやり方」をしてもうまくなんて行くわけもない。レビューに閉じた話じゃなくなりはじめてるし、この辺にしとこ。

どうでもいいけど「レビューイ」って変換しづらいし言いづらいし普段も滅多に使わない。最初聞いた時「レビュアー」が「ア」だから「アイウエオ」で「イ」なのかな?って思ってた。「レビュウ」とかあるんかな、とか。

まとめ

向き合ってるプロダクトやプロジェクト、メンバーなどいろんな要因で変わるもんです。なので「こうすべき」みたいな強い主張はないです。私個人としてニュートラルならこんな考え方だという話。なにもないなら主張することもあるけど、なんらか形があるときに押し付けたりはしない、くらいのものです。

「コメントしたら全部対処すべき」が一般的だと認識はしてる。コードレビューをゲートとして置くプロセスも理解できるし、それが立て付けられてるなら従ってます。それならそれで「もっとちゃんとやってこーぜ」と後押ししたりもする。「誰かが確認しないとマージしない」って暗黙のルールならちゃんとGitHubの設定そうしとこーぜ、とかね。そういう設定を勧めときながら個人としては好きじゃない、みたいなのは誰しもあると思います。その系です。

あ、タイトルの「対応する必要があるか」に対しては「対応しようよ。対応は別にコードの修正とかだけじゃなく、リアクションボタンだけでもいいからさ。」みたいな感じです。無視は辛い。的外れとか場違いとか不要なコメントをしてるとかなら無視でいいかもだけど、それも教えたげるのがいいと思う。「コメントは変更指示ではない」と言うのだけは言える。