Slide 1
Slide 1 text
RailsのPull requestsの
レビューの時に私が考えて
いること
Kaigi on Rails 2024
Oct 25, 2024
Yasuo Honda @yahonda
Slide 2
Slide 2 text
● Yasuo Honda / 本多康夫
○ Rails committer
○ Maintainer of Active Record Oracle enhanced adapter
○ https://github.com/yahonda
○ https://x.com/yahonda
○ https://mastodon.social/@yahonda
self
Slide 3
Slide 3 text
https://rubyonrails.org/community
Slide 4
Slide 4 text
No content
Slide 5
Slide 5 text
2024年10月24日時点
● 合計702コミット
● 500 : 自分がauthor
● 200 : Contributorsから
Rails Contributors
Slide 6
Slide 6 text
● Rails Nightly CI の安定化
○ https://buildkite.com/rails/rails-nightly
● Rails CIの安定化
○ https://buildkite.com/rails/rails
● RailsやRailsが依存するgemでの警告の修正
● bugs.ruby-lang.org へのissue報告
● だいたいの活動時間はこれで占められる
○ 「Kaigi on Rails 2024事後勉強会」で話します
日々やっていること
Slide 7
Slide 7 text
● Railsのissuesを見る
○ https://github.com/rails/rails/issues
● Railsのpull requestsを見る
○ https://github.com/rails/rails/pulls
● 今日はここを話します
その後にやっていること
Slide 8
Slide 8 text
Issuesについて
考えていること
Slide 9
Slide 9 text
● Reporting an Issue の内容を型として利用しましょう
● Issue template を型として利用しましょう
● Issueに書くこと
○ Title
○ Step to reproduce
○ Expected behavior
○ Actual behavior
○ System Configuration
Issues
Slide 10
Slide 10 text
● 問題のサマリーを書きましょう
○ Actual behaviorを1行でまとめるとよい
● 限られたTitleに [BUG] などのタグを書くのはもったいない
○ 問題があるからIssueを挙げているのは自明といえる
○ 貴重な文字数を問題の説明そのものに使った方がよい
Issues : Title
Slide 11
Slide 11 text
● あなたの問題が、他の人でも再現できるように書きましょう
○ Create an Executable Test Case (bundler/inline)が理想的
○ 事象が再現するソースコードレポジトリ + 再現手順が次善
● 再現手順(Steps to reproduce)ではないもの
○ コードスニペット
■ 誰かが不足した(明示されない)情報を補う必要がある
○ スタックトレース
■ さらに問題修正の難易度は上がる
Issues : Steps to reproduce
Slide 12
Slide 12 text
● あなたが望む動作を書く
○ Issueとfeature request(新機能)の区別は難しいことがある
■ 過去のバージョンでどう動いていたか
■ https://api.rubyonrails.org/ に記載されている振る舞いか
○ 新機能と判断されたらissueはクローズされる
■ What about Feature Requests?
■ rubyonrails-core - Ruby on Rails Discussions
○ 新機能を実現するpull requestを書くこともできます
Issues : Expected behavior
Slide 13
Slide 13 text
● 再現ケースで起きる実際の動作を書く
○ 実際の動作なので肯定文で書く
○ 例: “It raises an ArgumentError…”など
● 否定文だと必要な情報が追加されない
○ 例: “It does not work.” “Not working”
○ Expected behaviorではないこと以上がわからない
Issues : Actual behavior
Slide 14
Slide 14 text
● 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の問題や不具合を管理するためにある
Issues : System configuration
Rails version
Slide 15
Slide 15 text
● Issueが発生するRubyのバージョン
○ $ ruby -v の出力をかく
■ 例: ruby 3.3.5 (2024-09-03 revision ef084cc8f4)
[x86_64-linux]
■ “Ruby 3”ではどのバージョンかわからない
○ 可能なら複数のRubyバージョンで再現するか、あるいは特定のバー
ジョンでのみ再現する、しないなどの情報はなおよい
Issues : System configuration
Ruby version
Slide 16
Slide 16 text
Pull requestsについて
考えていること
Slide 17
Slide 17 text
● Contributing to the Rails Code の内容を型として利用しましょう
● Pull request template を型として利用しましょう
● Pull Requestに書くこと
○ コードとテスト、コミットメッセージ
○ Title
○ Motivation / Background
○ Detail
○ Additional information
○ Checklist
Pull requests
Slide 18
Slide 18 text
● Pull request templateは型として利用しましょう
○ 型がなくてもpull requestの必要性を説明できる場合は、必ずしも、
テンプレートに従う必要はありません
○ No description provided. のpull requestには何もしません
○ 私のpull requestはテンプレートに従っています
■ コミットメッセージの後にテンプレートが来る
■ 編集がもう少し楽になればいいのにと思うこともある
Pull request template
Slide 19
Slide 19 text
● Active Record
○ ConnectionAdapters
○ Migration
○ データベースに特化した機能(PostgreSQL/MySQL/SQLite)
● RailtiesやActive Support
○ わかるところだけ
私がレビューしている pull requestの分野
Slide 20
Slide 20 text
● 標準でSQLite, MySQL, PostgreSQLの3つのデータベースに対応
○ データベース共通機能と特化機能のバランス
○ 特定のデータベースのみに存在する機能は入りにくい
● データベースの機能を全てカバーすることは意図していない
○ Railsのアプリケーションに必要な機能かどうか
○ MigrationのDSLで表現できなくても、execute メソッドでRaw SQL
が実行する手法がある
ConnectionAdaptersの特徴
Slide 21
Slide 21 text
● すぐにマージできるもの : 数は多い
● マージに向けてレビューした方がいいと思うもの
○ 主に内容のレビューに時間がかかるため、数は少ない
● pull requestの方針に疑問、反対の意見を持っているもの
○ 質問をしたり、反対の意見を述べたり、クローズしたりする
○ 主に作文に時間がかかるため、数は少ない
● どちらでもよいもの
○ 何もしない
○ 現実的にはこれが最も多い(限られたリソース)
Pull requestをレビューするかの観点
Slide 22
Slide 22 text
● いちばん重要な要素
○ Real world use case?と聞かれた人もいると思います
○ あなたのユースケースであることが重要
■ 理由を説明できるから
● ユースケースの種かも知れないが、ユースケースではまだないもの
○ データベースに新機能が追加された
■ 利用する全ての技術要素の対応を目的にしていないから
○ 魅力的な技術の場合は、例外もありうる
1. ユースケースがあるか
Slide 23
Slide 23 text
● https://github.com/rails/rails/pull/52285
● 実際のユースケースがあるかと質問したが、特にないとのこと
○ ユースケースがある方は上記pull requestにコメントください
● ユースケースなしでマージすると、それが将来の類似pull requestを
マージする理由になってしまう
● PostgreSQLバージョンによる分岐を追加したくなかった
○ RailsはPostgreSQL 9.3以上に対応
○ 小さな変更だからマージすればいいのではというコメントもあったが、
コードが複雑する一歩である
例:PostgreSQL 10+でのmacaddr8データ型
Slide 24
Slide 24 text
● Pull request作者のユースケースがあった上で、それが多くのユーザー
にとって役立つのかを判断する必要がある
○ マージされたコードはメンテナンスする必要がある
○ Railsに追加された機能を削除するには、最短でも2マイナーバージョ
ンが必要
■ Deprecation warningサイクルの後に削除するため
2.ユースケースは多くのユーザーに役立つか
Slide 25
Slide 25 text
● https://github.com/rails/rails/pull/45776
● ユースケースは文字列をcase insensitiveで検索したいとのこと
● 反対した理由
○ 他の手段がある uniqueness: { case_sensitive: false }, citext
○ create collationで照合順序を作成できるのはPostgreSQLだけ
○ executeメソッドでSQL文(create collation)実行もできる
○ ユーザーが照合順序を作成は回数はおそらく1桁
● 追加する意思がないことを伝えてクローズ
例:MigrationでPostgreSQLのcollation作成
Slide 26
Slide 26 text
● Railsは他のソフトウェアと共に用いられる
○ 問題を解決すべきレイヤーで解決したい
● 各ユーザーがをmonkey patchできることのメリット、デメリット
○ メリット : ユーザーがquick fixできる
○ デメリット : 副作用の発生、問題解決すべきレイヤーの見逃し
● Railsが利用するソフトウェアの開発体制を知るのも良い経験
3.そのユースケースは Railsで解決すべきか
Slide 27
Slide 27 text
● 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にするという内容
例:pg_stat_statementsの”汚染”
Slide 28
Slide 28 text
● このpull requestへの賛成のコメントが複数あった
○ 複数ユーザーからのユースケース自体はあるといえる
● 明確に反対した
○ pg_stat_statementsユーザーのユースケースの解決のために、全
ユーザーが発行するSQLを書き換えるのはバランスが悪い
○ 発行されるSQLが変わる(INからANY)のは望ましい変化ではない
○ この問題はPostgreSQL側での解決が図られてほしい
例:pg_stat_statementsの”汚染”
Slide 29
Slide 29 text
● マージに必要なアクションのアドバイスをもらう
○ 第46回 PostgreSQLアンカンファレンス@東京
○ pg_stat_statementsで inの数が違うSQLをまとめて ほしい
● pgsql-hackersメーリングリストにRailsのユースケースを投稿
● patchを当てたPostgreSQLをビルドして動作確認のコメントなど
● ちょっとずつ反応はもらうがまだマージには至らず
○ Commitfest(パッチのレビュー期間)ではMoved to next CF
○ https://commitfest.postgresql.org/50/2837/
例:pg_stat_statementsの”汚染”
Slide 30
Slide 30 text
● Railsのleaky abstraction(漏れのある抽象化)
○ RailsConf 2018: Opening Keynote: FIXME by David
Heinemeier Hansson
● RailsのMigrationは、DDLが透けて見えるような実装
○ 複雑なSQLの全ての文法には対応できない
○ SQL生成をするDSLの実装/APIの考慮は難しい
4.適切な抽象化をしているか
Slide 31
Slide 31 text
● 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と振る舞いの違いがよくわからない
例:PostgreSQLでunique制約を遅延
Slide 32
Slide 32 text
● trueと:deferredは発行されるSQL は違うが効果は同じとわかる
○ 第39回 PostgreSQLアンカンファレンス@オンライン
○ 遅延可能な一意性制約
● 遅延可能なunique制約の追加時には、deferrable: の引数
に:deferred, :immediateの2つのみ取れるようにした
○ この機能のユーザーはPostgreSQLの遅延制約について理解があ
り、:deferredと:immediateの設定は許容されると判断
● 遅延可能なforeign key制約の追加でも、true をdeprecatedにした
○ https://github.com/rails/rails/pull/47659
例:PostgreSQLでunique制約を遅延
Slide 33
Slide 33 text
そのほか考えること
Slide 34
Slide 34 text
● ほとんどのPull requestは既存の行の更新、削除を含みます
● 変更したいコードも、過去なんらかの理由で書かれたもの
○ コミットメッセージから、現状の理由を把握し、その上で今この行を更
新するのが妥当なのか考える
○ コードフォーマットなどの変更は飛ばして、意味のある変更がされたコ
ミットメッセージを読む
● これから書くコードも、将来他の誰かが変更するかも知れません
○ その時の判断基準となるコミットメッセージを書きましょう
Git blameで変更したい行の commitを読む
Slide 35
Slide 35 text
● Issueやpull request へのコメントが :+1: で埋められる解消策
● レビュー時にあまり参考にしていない( +1 以上でも以下でもない)
○ 数が多いからといってマージするわけではない
○ 多すぎるEmojiリアクションは、”組織票”の可能性を考える
○ 多くのユーザーにとって必要なら、ひろくの反応があるはず
● あなたのユースケースがあるなら、それを文章にしてコメントした方が良い
Emoji
Slide 36
Slide 36 text
● Pull requestを作ってくれたことへの感謝の気持ちを文章に表す
● その上で、マージしないと判断するのも必要
● クローズする時には十分な時間をかけ理由を説明する
○ 議論を長引かせないため、はっきり根拠と意思は示す
Pull requestをクローズする時
Slide 37
Slide 37 text
● Pull requestやIssueが読まれるために必要なスキル
○ Title,Descriptionも自然言語のみ
○ GitHubのFiles changedタブまで、ソースコードは出てこない
● Grammar in Use（アメリカ英語）
○ 助動詞の意図とかとても参考になりました
● 2020年代の今、生成AIや機械翻訳は有用なツール
○ 最終的に記述する英語の意味が自分でもわかる必要はある
英語
Slide 38
Slide 38 text
さいごに
Slide 39
Slide 39 text
● やりたいことがあったら、Contributing to Ruby on Rails を読んで、や
れることをやって、pull requestをだしましょう
● First time-contributor ラベルのあるpull requestは意識して見てい
ます
将来のFirst contributorの皆さんへ
Slide 40
Slide 40 text
End