1 of 40

RailsのPull requestsのレビューの時に私が考えていること

Kaigi on Rails 2024

Oct 25, 2024

Yasuo Honda @yahonda

2 of 40

self

  • Yasuo Honda / 本多康夫

3 of 40

https://rubyonrails.org/community

4 of 40

5 of 40

Rails Contributors

2024年10月24日時点

  • 合計702コミット
  • 500 : 自分がauthor
  • 200 : Contributorsから

6 of 40

日々やっていること

  • Rails Nightly CI の安定化
  • Rails CIの安定化
  • RailsやRailsが依存するgemでの警告の修正
  • bugs.ruby-lang.org へのissue報告
  • だいたいの活動時間はこれで占められる
    • 「Kaigi on Rails 2024事後勉強会」で話します

7 of 40

その後にやっていること

  • Railsのissuesを見る
  • Railsのpull requestsを見る
  • 今日はここを話します

​

8 of 40

Issuesについて

考えていること

9 of 40

Issues

  • Reporting an Issue の内容を型として利用しましょう
  • Issue template を型として利用しましょう
  • Issueに書くこと
    • Title
    • Step to reproduce
    • Expected behavior
    • Actual behavior
    • System Configuration

​

10 of 40

Issues : Title

  • 問題のサマリーを書きましょう
    • Actual behaviorを1行でまとめるとよい
  • 限られたTitleに [BUG] などのタグを書くのはもったいない
    • 問題があるからIssueを挙げているのは自明といえる
    • 貴重な文字数を問題の説明そのものに使った方がよい

​

11 of 40

Issues : Steps to reproduce

  • あなたの問題が、他の人でも再現できるように書きましょう
    • Create an Executable Test Case (bundler/inline)が理想的
    • 事象が再現するソースコードレポジトリ + 再現手順が次善
  • 再現手順(Steps to reproduce)ではないもの
    • コードスニペット
      • 誰かが不足した(明示されない)情報を補う必要がある
    • スタックトレース
      • さらに問題修正の難易度は上がる

​

12 of 40

Issues : Expected behavior

  • あなたが望む動作を書く
    • Issueとfeature request(新機能)の区別は難しいことがある
      • 過去のバージョンでどう動いていたか
      • https://api.rubyonrails.org/ に記載されている振る舞いか
    • 新機能と判断されたらissueはクローズされる
    • 新機能を実現するpull requestを書くこともできます

​

13 of 40

Issues : Actual behavior

  • 再現ケースで起きる実際の動作を書く
    • 実際の動作なので肯定文で書く
    • 例: “It raises an ArgumentError…”など
  • 否定文だと必要な情報が追加されない
    • 例: “It does not work.” “Not working”
    • Expected behaviorではないこと以上がわからない

14 of 40

Issues : System configuration

Rails version

  • Issueが発生するRailsのバージョン($ bundle exec rails -v)
    • Maintenance Policy for Ruby on Rails にある
      • New Features / Bug Fixesに含まれるバージョンである
    • Security Issues、End-of-life Release Seriesの場合
      • Bug Fix提供されないバージョンのissueは、Bug Fixesに含まれるバージョンにあげて問題が再現する必要がある
      • IssueはRailsの問題や不具合を管理するためにある

15 of 40

Issues : System configuration

Ruby version

  • Issueが発生するRubyのバージョン
    • $ ruby -v の出力をかく
      • 例: ruby 3.3.5 (2024-09-03 revision ef084cc8f4) [x86_64-linux]
      • “Ruby 3”ではどのバージョンかわからない
    • 可能なら複数のRubyバージョンで再現するか、あるいは特定のバージョンでのみ再現する、しないなどの情報はなおよい

16 of 40

Pull requestsについて

考えていること

17 of 40

Pull requests

  • Contributing to the Rails Code の内容を型として利用しましょう
  • Pull request template を型として利用しましょう
  • Pull Requestに書くこと
    • コードとテスト、コミットメッセージ
    • Title
    • Motivation / Background
    • Detail
    • Additional information
    • Checklist

​

18 of 40

Pull request template

  • Pull request templateは型として利用しましょう
    • 型がなくてもpull requestの必要性を説明できる場合は、必ずしも、テンプレートに従う必要はありません
    • No description provided. のpull requestには何もしません
    • 私のpull requestはテンプレートに従っています
      • コミットメッセージの後にテンプレートが来る
      • 編集がもう少し楽になればいいのにと思うこともある

​

19 of 40

私がレビューしているpull requestの分野

  • Active Record
    • ConnectionAdapters
    • Migration
    • データベースに特化した機能(PostgreSQL/MySQL/SQLite)
  • RailtiesやActive Support
    • わかるところだけ

20 of 40

ConnectionAdaptersの特徴

  • 標準でSQLite, MySQL, PostgreSQLの3つのデータベースに対応
    • データベース共通機能と特化機能のバランス
    • 特定のデータベースのみに存在する機能は入りにくい
  • データベースの機能を全てカバーすることは意図していない
    • Railsのアプリケーションに必要な機能かどうか
    • MigrationのDSLで表現できなくても、execute メソッドでRaw SQLが実行する手法がある

21 of 40

Pull requestをレビューするかの観点

  • すぐにマージできるもの : 数は多い
  • マージに向けてレビューした方がいいと思うもの
    • 主に内容のレビューに時間がかかるため、数は少ない
  • pull requestの方針に疑問、反対の意見を持っているもの
    • 質問をしたり、反対の意見を述べたり、クローズしたりする
    • 主に作文に時間がかかるため、数は少ない
  • どちらでもよいもの
    • 何もしない
    • 現実的にはこれが最も多い(限られたリソース)

22 of 40

  1. ユースケースがあるか
  • いちばん重要な要素
    • Real world use case?と聞かれた人もいると思います
    • あなたのユースケースであることが重要
      • 理由を説明できるから
  • ユースケースの種かも知れないが、ユースケースではまだないもの
    • データベースに新機能が追加された
      • 利用する全ての技術要素の対応を目的にしていないから
    • 魅力的な技術の場合は、例外もありうる

23 of 40

例:PostgreSQL 10+でのmacaddr8データ型

  • https://github.com/rails/rails/pull/52285
  • 実際のユースケースがあるかと質問したが、特にないとのこと
    • ユースケースがある方は上記pull requestにコメントください
  • ユースケースなしでマージすると、それが将来の類似pull requestをマージする理由になってしまう
  • PostgreSQLバージョンによる分岐を追加したくなかった
    • RailsはPostgreSQL 9.3以上に対応
    • 小さな変更だからマージすればいいのではというコメントもあったが、コードが複雑する一歩である

​

24 of 40

2.ユースケースは多くのユーザーに役立つか

  • Pull request作者のユースケースがあった上で、それが多くのユーザーにとって役立つのかを判断する必要がある
    • マージされたコードはメンテナンスする必要がある
    • Railsに追加された機能を削除するには、最短でも2マイナーバージョンが必要
      • Deprecation warningサイクルの後に削除するため

25 of 40

例:MigrationでPostgreSQLのcollation作成

  • https://github.com/rails/rails/pull/45776
  • ユースケースは文字列をcase insensitiveで検索したいとのこと
  • 反対した理由
    • 他の手段がある uniqueness: { case_sensitive: false }, citext
    • create collationで照合順序を作成できるのはPostgreSQLだけ
    • executeメソッドでSQL文(create collation)実行もできる
    • ユーザーが照合順序を作成は回数はおそらく1桁
  • 追加する意思がないことを伝えてクローズ

26 of 40

3.そのユースケースはRailsで解決すべきか

  • Railsは他のソフトウェアと共に用いられる
    • 問題を解決すべきレイヤーで解決したい
  • 各ユーザーがをmonkey patchできることのメリット、デメリット
    • メリット : ユーザーがquick fixできる
    • デメリット : 副作用の発生、問題解決すべきレイヤーの見逃し
  • Railsが利用するソフトウェアの開発体制を知るのも良い経験

27 of 40

例:pg_stat_statementsの”汚染”

  • https://github.com/rails/rails/pull/49388
  • PostgreSQLのpg_stat_statementsモジュール
    • 実行されたSQLの統計情報を記録(デフォルト5000)
    • INの引数の数が異なるSQLが異なるレコードとして記録される
    • 類似SQLでpg_stat_statementsが埋まる(pollution)ことも
  • ANY : 引数が異なってもpg_stat_statements上同一SQLとなる
    • Pull requestは、Arelのvisit_Arel_Nodes_HomogeneousIn メソッドを書き換えて、INをANYにするという内容

28 of 40

例:pg_stat_statementsの”汚染”

  • このpull requestへの賛成のコメントが複数あった
    • 複数ユーザーからのユースケース自体はあるといえる
  • 明確に反対した
    • pg_stat_statementsユーザーのユースケースの解決のために、全ユーザーが発行するSQLを書き換えるのはバランスが悪い
    • 発行されるSQLが変わる(INからANY)のは望ましい変化ではない
    • この問題はPostgreSQL側での解決が図られてほしい

29 of 40

例:pg_stat_statementsの”汚染”

  • マージに必要なアクションのアドバイスをもらう
  • pgsql-hackersメーリングリストにRailsのユースケースを投稿
  • patchを当てたPostgreSQLをビルドして動作確認のコメントなど
  • ちょっとずつ反応はもらうがまだマージには至らず
    • Commitfest(パッチのレビュー期間)ではMoved to next CF
    • https://commitfest.postgresql.org/50/2837/

30 of 40

4.適切な抽象化をしているか

  • Railsのleaky abstraction(漏れのある抽象化)
  • RailsのMigrationは、DDLが透けて見えるような実装
    • 複雑なSQLの全ての文法には対応できない
    • SQL生成をするDSLの実装/APIの考慮は難しい

​

31 of 40

例:PostgreSQLでunique制約を遅延

  • https://github.com/rails/rails/pull/46192
  • 納得できるユースケースが説明されていた
  • すでにforeign keyを遅延可能になっていた
    • https://github.com/rails/rails/pull/41487
    • 遅延可能なforeign keyの実装に倣って作成
  • 疑問 : deferrable: の引数に:deferred, :immediate, true の3種類
    • ableで終わるので、true / false の二択を連想させるAPI
    • 異なる値ごとの3つのSQLと振る舞いの違いがよくわからない

32 of 40

例:PostgreSQLでunique制約を遅延

  • trueと:deferredは発行されるSQL は違うが効果は同じとわかる
  • 遅延可能なunique制約の追加時には、deferrable: の引数に:deferred, :immediateの2つのみ取れるようにした
    • この機能のユーザーはPostgreSQLの遅延制約について理解があり、:deferredと:immediateの設定は許容されると判断
  • 遅延可能なforeign key制約の追加でも、true をdeprecatedにした

33 of 40

そのほか考えること

34 of 40

Git blameで変更したい行のcommitを読む

  • ほとんどのPull requestは既存の行の更新、削除を含みます
  • 変更したいコードも、過去なんらかの理由で書かれたもの
    • コミットメッセージから、現状の理由を把握し、その上で今この行を更新するのが妥当なのか考える
    • コードフォーマットなどの変更は飛ばして、意味のある変更がされたコミットメッセージを読む
  • これから書くコードも、将来他の誰かが変更するかも知れません
    • その時の判断基準となるコミットメッセージを書きましょう

35 of 40

Emoji

  • Issueやpull request へのコメントが :+1: で埋められる解消策
  • レビュー時にあまり参考にしていない( +1 以上でも以下でもない)
    • 数が多いからといってマージするわけではない
    • 多すぎるEmojiリアクションは、”組織票”の可能性を考える
    • 多くのユーザーにとって必要なら、ひろくの反応があるはず
  • あなたのユースケースがあるなら、それを文章にしてコメントした方が良い

36 of 40

Pull requestをクローズする時

  • Pull requestを作ってくれたことへの感謝の気持ちを文章に表す
  • その上で、マージしないと判断するのも必要
  • クローズする時には十分な時間をかけ理由を説明する
    • 議論を長引かせないため、はっきり根拠と意思は示す

​

37 of 40

英語

  • Pull requestやIssueが読まれるために必要なスキル
    • Title,Descriptionも自然言語のみ
    • GitHubのFiles changedタブまで、ソースコードは出てこない
  • Grammar in Use（アメリカ英語）
    • 助動詞の意図とかとても参考になりました
  • 2020年代の今、生成AIや機械翻訳は有用なツール
    • 最終的に記述する英語の意味が自分でもわかる必要はある

38 of 40

さいごに

39 of 40

将来のFirst contributorの皆さんへ

  • やりたいことがあったら、Contributing to Ruby on Rails を読んで、やれることをやって、pull requestをだしましょう
  • First time-contributor ラベルのあるpull requestは意識して見ています

40 of 40

End