社内でPull Requestsを運用する時の俺プラクティス

gistfile1.md

※2018/11/25 この考えはもういろいろ間違っていると思うのでそのうち書き直す

最近レビュー業ばかりしているので、ある程度思ったことを書くよ。鵜呑みは良くないよ。ベストプラクティスとは言ってないよ。 あと所属企業とは何も関係ないよ。あとOSSでは参考にならないよ。

レビューは相手に優しく

もちろん「駄目。やり直し」などは論外。そんなのはレビューとは言わない。 相手も人間なので正論すぎるとイラッと来る可能性がある。常に相手に優しくしよう。それができないならどれだけ技術力があってもレビュワーには向いていない。（OSSなら好きにすればいいけど） 相手が新卒だからと言ってつい上から目線になるのも良くない。人によっては怖がって、最悪の場合辞めてしまう可能性もあるので、なるべく怖がらせないようにしよう。（その程度で辞めるような奴はいらん！というモヒカン会社ならご自由に） 逆にレビューされる側はどんどんツッコミを入れよう。「えっそんな修正必要なんですか？」とか「作業効率に対してその修正はどのくらいメリットあるの？馬鹿なの？死ぬの？」とかどんどん攻めていい。どうしてもレビューする側のほうが立場が上に見えることが多々あるので、レビューされる側は攻めの姿勢で。

レビュー内容は一緒に考える

やはり「駄目。やり直し」は論外として、理由を語らずに指摘するのは基本的にNG。指摘する側が理由の本質を説明できない場合にこのような状況が発生するからである。また、理不尽な指摘だと思われる可能性が高い。 いきなり回答を出したくないのであれば、少なくとも「可読性に欠けるのでNG」とか「処理効率が悪すぎるのでNG」とかは最低でも書くべきである。 また、駄目なら駄目で、多少の提案のヒントは与えるべきだと思っている。駄目だと感じたのであればその時点で改善方法が頭に浮かんでいるから書くのは難しくないだろう。改善方法が浮かばないならそれはレビューではなくただの愚痴でしかない。

コードの意味を使い分ける

コードには「美しいコード」「動作が最速のコード」「仕様的に正しいコード」「ビジネス的に正しいコード」等がある。 基本的にはどれも同じだが、たまにこれが違う場合というものが存在する。 例えばソートする時に組み込みのqsortを使うか、ライブラリを持ってくるか、自前で最適なオーダーになるソートを書くか、といった感じである。仮に自前で書くと明らかな差が出るとしても、それがあまり使われない処理であれば無理に高速化する必要はない。 仕事でコードを書く以上、利益を上げるのが最大の目的なので、そこはぶれないようにしよう。

変な用語はなるべく避ける・どうしても使う場合は社内で必ず浸透させる

個人的にはWIPまでは許せるけど（それでも知らないって言う人は結構多い）、IMOとか全然聞いたことがない。 ここは日本語圏だ。英語圏ではない。無理に英語略を使う必要はない。（社内言語が英語だって言うならいいけど） 私ならそう思う、なら「私感」と書けばいい。そのほうがほとんどの日本人にはわかりやすい。 逆に社内で浸透できるなら好き勝手に用語を増やせばいいが、新しい人が入ってきてわかりづらいと困るので、ほどほどにしたほうがいい。「その用語を使うことでどれだけ効率が上がるか」を考えたほうがいい。

作業中のPRでもどんどん送る

とにかく送ろう。早期でレビュワーが何か拾ってくれるかもしれない。 この時に頭に[WIP]とか書くのがおすすめだが、WIPってなんじゃそりゃって思うのであれば[作業中]でも全然構わない。日本人なんだから無理に英語を使うのはやめよう。ださい。英語のほうがtype数は少ないけど。 ちなみにWIPと書くとマージできなくなるChrome拡張も存在する（https://chrome.google.com/webstore/detail/github-merge-caution/nimelepbpejjlbmoobocpfnjhihnpked）

マージは作業者が行ってはいけない

人が少ないなら仕方ないが、マージというのは作業完了＝レビューOKという意思表示でもあるので、原則的に作業者は行わないほうがいい。 主観というのは100%信頼できないので、必ず他人がマージするようにしよう。 作った人がマージしないといつリリースするんだよ！というのもあるが、通常マージとリリースタイミングは切り離せるはずである。

PRに関すること以外はよそでやろう

github限定の話だけど、とにかくgithubのissue周りはゴミだと思ってるので、例えば仕様に関する議論をそこで行ったりすると、後から調べるのに苦労する。 コードレビュー以外のことは他のツールを使うべきである。コードレビュー以外にgithubは全く向いていない。 特に仕様に関しては後から書き換えられる可能性のあるものなので、参照性の高い場所に置いておくべきである。間違ってもPRと一緒に置いといてはいけない。PR（issue）は消えるもの、と思ったほうがいい。そのくらい検索性が悪い。

まとめ

• レビューする側、される側、どちらも基本的には同じ立場だと思って作業しよう。いつ立場が逆転してもおかしくない。
• githubのissue周りはゴミ