初心者向けのPull Request（PR）作成方法はopensource.guideをはじめとして数多く見るのですが、もうちょっと突っ込んだというか中級的な内容の記事を読みたかったので自分で書きます。題材として主に直近で書いた大きめの機能追加用PRを使っています：
修正や新機能を入れる利点を明確に伝える
何かを提案する場合、その背景には必ず動機となる利点があるはずです。これはとても単純に伝えられることもあれば、テストやグラフを作成しないと伝わりにくいものもあります。
今回の変更では、GitHub Actions workflowの定義ファイル簡素化とパフォーマンス改善が利点でした。定義ファイルが簡素化されることはドキュメントで簡単に伝えられますが、パフォーマンスは測定しなければわかりません。よってパフォーマンス検証用のリポジトリを作成し測定を行うことで、ビルド時間が約28%高速化されることを示しました。
このリポジトリにあるActionsを実行すると、グラフを描くために必要なCSVが出力されます。それをGoogle Sheetsに突っ込んで以下のようなグラフを作成し、PRページに添付しています。結果として、本当に高速化に繋がるのか？といった疑問や質問は来ず、純粋に実装手法についての議論から入ることができました。
こうして根拠となる事実を自動的に出せるようにしておくと、レビューを受けて実装が変わったときに再確認を行えるという利点もあります。今回の例で言うとキャッシュの解決とJDKのインストールとを並行することでの高速化を意図していたのですが、セキュリティ上の兼ね合いから順次実行するようコードを変更せざるを得ませんでした。このような大きな変更があっても、既に自動化されたパフォーマンス比較手段があったため、パフォーマンス改善が維持できていることを示せました。
考慮すべきことも伝える
変更には利点だけではなく、互換性や運用など考慮しなければならないこともあります。これらをどこまで認識していて、どのような検証を行ったのかを事前に共有することで、多角的な議論の下地とすることができます。
今回の変更では upload-chunk-size が設定できなくなりユーザの自由度が失われること、IvyやGrapeといったより多くのパッケージマネージャをサポートすべきかもしれないことを俎上に載せ、その上でそれらが現時点で問題にならない理由を述べています。
新機能をメンテナンスし続けることのコストを最小化する
新機能追加のためにPRを送る場合、その機能のメンテナンスを行うのが自分ではなく相手だという点を意識する必要があります。言い換えれば、メンテナンスの工数が高いと判断された場合、PRをマージしてもらえない可能性があるということです。
基本的には単体テストと統合テストの双方を実装し提供することで対応します。単体テストがあることで実装自体のテスト可能性を担保し、統合テストがあることでリグレッションに気づける状態を作ります。またユーザに対する説明が必要な機能なら、READMEのような書類もあわせて準備します。
また自己流のやり方を持ち込むのではなく、既存のコードベースに溶け込むコードを書くことも重要です。今回は既にある定数クラスを再利用したり、統合テストのための仕組みに乗ったりという工夫をしました。特に統合テストのやり方はリポジトリによって様々なので、既存のビルド設定やコードに目を通す必要があります。
早めに返信するが、相手には求めない
質問や指摘があった場合、24時間以内には必ず何らかのリアクションを返すようにします。どうせタイムゾーンの問題があるので”即レス”は難しいのですが、できるだけ早めに返すことで議論の沈静化を防ぎます。
逆に、向こうから連絡がなくても気長に待ったほうが良いことも多いです。今回は企業の中の人が業務としてレビューしてくれましたが、それでも2週間ほど返信がなかった期間がありました。自分が相手の立場でもそういう事はあるだろうと気長に待つのが得策です。
変更の理由をオーバー気味に説明する
PRを提案する際、依存先の更新やリファクタリングのような、一見不要に見える変更を入れることがあります。例えばこちらのPRでは、ネットワークエラー発生時でもHTTPリクエストのリトライを行うために typed-rest-client のバージョンを上げています。何も説明しないと、依存を上げるのは他のPRでやればいいのでは？という疑問が湧くかもしれません。
こうした変更を入れる際は、コミットコメントかPR内コメントでその意図を伝えるようにしています。やはりコードでWHYを伝えるのは難しいので、少しオーバーかなと思うくらいの勢いで伝えたほうが良さそうです。
コードには How
— Takuto Wada (@t_wada) 2017年9月5日
テストコードには What
コミットログには Why
コードコメントには Why not
を書こうという話をした
その他、細かいこと
README.mdはもちろん、CONTRIBUTING.mdやCODE_OF_CONDUCT.mdといったドキュメントに目を通しましょう。- Pull Requestテンプレートがある場合は利用しましょう。
- わからないことはわからないと言い、素直に尋ねる方が良いです。相手もこちらを助けやすくなります。
- 新しいライブラリの導入など相手に判断を求めるケースでも、自分の考えを述べた上で素直に尋ねるようにしましょう。
以上です。どなたかの参考になれば幸いです。 Happy hacking!
