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

https://kaigionrails.org/2024/talks/yahonda/

リンクがクリックできるGoogle Docs:
https://docs.google.com/presentation/d/12HPYizn1ZZwilG4ivwX4ZO35AOdigBTszNZKYGa9eLE/edit#slide=id.g160791fb4a6_0_0

Transcript

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