Establish reviewing process

[smikitky]

Now that many people are interested in review and we have many active PRs, we should establish a written rule regarding the review process.

• Who will take responsibility in the initial review process?
• When a maintainer (@koba04 @potato4d @koba04) can decide to merge?

[smikitky]

個人的にはファーストラウンドが終わったらあとは同時平行になっても大丈夫かなと思います。

ラベルについては、"in review" だと曖昧すぎるので "in initial review" / "initial review underway" / "初回レビュー中" あたりでどうでしょう。

1. 初回レビュアーが宣言
2. 初回レビュアーレビュー（数時間以内には終えること）
3. 作業者レビュー対応
4. 1～3までは守る（ただし個々の細かいツッコミは誰でも適宜OK）が、初回レビューラウンドが終わったら後は誰でも同時にレビューOK、approve 権限がある（＝リポジトリにwriteアクセスがある）日本語話者（ @koba04 @potato4d @smikitky の3名）のうち 2 名の approve によりマージ

[sasurau4]

@smikitky
1-4のレビューフロー、良いと思います 👍
1点質問です。

1. in initial review / initial review underway / 初回レビュー中 のラベルは1-3の間はずっと付いていて、4に入ったら外すという運用イメージですか？

[smikitky]

@sasurau4 大量の重複したレビューがつくのを防ぐというのが最大の目的ですので、2が終わったらもう外していいかなと思います。

[koba04]

では、一旦そのような形で運用してみましょうか。
問題が発生したらまたこちらで議論しましょう。

ラベルに関しては、特にこだわりはありませんが、"in initial review"でどうでしょうか。

[sasurau4]

今気づいたんですが、write権限ないとラベルつけたり剥がしたりできないですね...
https://help.github.com/articles/repository-permission-levels-for-an-organization/

[smikitky]

はい、現時点では @potato4d @koba04 @smikitky （とメインリポジトリ側の方数名）のみが write 権限の所有者です（この3名は管理権限もあります）。管理権限の write access を一部の人に付与することも可能ですが、今のところはこの3人で大丈夫そうな気がしています。

[sasurau4]

@smikitky では、レビューより翻訳を進める方で協力していきます 👍

[smikitky]

翻訳PRをどうマージするかについて。

現状ではGitHubのUI上でsquash-mergeするというルールだと思いますが、これをすると、master側でtextlintのルール変更があった場合に、マージ後に master上でCIのエラーが出ることで気付く、ということが起こります。（例 #101 と #116)

対策としては以下のようなものが挙がりますが、どうしましょうか。

1. 普通に squash-merge してPRをクローズした後に、メンテナ側でエラーを修正する commit を master に直接 push する（例： aed3392 ）
   • 👍 とにかく速くて楽
   • 👎 masterに❌つきコミットが残る
2. PRのマージをローカルのコマンドラインで行い、修正してからpushすることで、エラーが出るとわかりきっているコミットに対して自動CIが走らないようにする
   • 👍 比較的速い
   • 👎 何にせよ一瞬masterにエラーのあるコミットは残る・squashマージにするとGitHub側がマージされたことを認識できない（はず）
3. マージ前に PR 側に master をマージすることを強制する（GitHubの"Require branches to be up to date before merging"オプションを有効にする。PR上でグリーンにならないとマージできない）
   • 👍 masterには綺麗な squash 済みコミットだけが残る
   • 👎 翻訳者側の負荷増とそれによるマージの遅れ・Sync with reactjs.org @ f767be60 #118 のような sync 用のPRでまでこれをやりたくないので sync は admin 権限がある人がルールを無視して強制的に行う必要がある・GitHub上での差分表示でちょっと混乱するかも・textlintのルール変更はそこまで頻繁には起きないので将来的にはあまり重要でないステップになるかも
4. マージ前に master へのリベースを強制する
   • 👍 masterには綺麗な squash 済みコミットだけが残る
   • 👎 翻訳者側の負荷増・信念としてリベースが嫌いな人がいそう

個人の意見ですが、1が一番楽な気がしています。マージ後に見つかるようなエラーは殆どが textlint の細かいエラーであり合意形成というほどのこともありません。翻訳者側には git 自体に不慣れな人もいます。「masterをマージしたらエラーが出ていますので修正をお願いします」などとお願いしてわかりきった作業のために1日待つよりは、メンテナ側で1分で修正した方が早いです。masterに❌が残ることには少なくとも美観上の問題はありますが、自動デプロイなどは❌がある状態では起きないようにすればいいだけなので、本質的には問題ではないように思います。

（なお、もちろん原文に大きな変更がある場合など確実に大きな不整合が予想される場合は rebase や git merge master を併用していくべきなで、あくまで PR に master をマージすることを全PRで強制まではしなくてもいいのでは、という話です）

[potato4d]

1. で良いと思います。 4. をやってくれる翻訳者は翻訳者側でやってくれるはずなので、意識しないでよいかと思います。
Deno がそんな感じの運用をしているみたいで、良さそうだなーと思ったりしました。

http://hashrock.hatenablog.com/entry/2019/02/04/040505

[koba04]

個人的には今のフローでも特に問題は感じていないですが(SyncのPRのmergeは確かに微妙)、1でも大丈夫です。
1の場合、merge後のCIを必ずチェックして対応する必要があるので、モバイルからmergeしずらいなと思ったのですが、approveだけして後でmergeは後からすればいいので大丈夫そうです。