Upgrade to PRO for Only $50/Year—Limited-Time Offer! 🔥

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

Yasuo Honda
October 25, 2024

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

Yasuo Honda

October 25, 2024
Tweet

More Decks by Yasuo Honda

Other Decks in Programming

Transcript

  1. • Yasuo Honda / 本多康夫 ◦ Rails committer ◦ Maintainer

    of Active Record Oracle enhanced adapter ◦ https://github.com/yahonda ◦ https://x.com/yahonda ◦ https://mastodon.social/@yahonda self
  2. • 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事後勉強会」で話します 日々やっていること
  3. • Reporting an Issue の内容を型として利用しましょう • Issue template を型として利用しましょう •

    Issueに書くこと ◦ Title ◦ Step to reproduce ◦ Expected behavior ◦ Actual behavior ◦ System Configuration Issues
  4. • 問題のサマリーを書きましょう ◦ Actual behaviorを1行でまとめるとよい • 限られたTitleに [BUG] などのタグを書くのはもったいない ◦

    問題があるからIssueを挙げているのは自明といえる ◦ 貴重な文字数を問題の説明そのものに使った方がよい Issues : Title
  5. • あなたの問題が、他の人でも再現できるように書きましょう ◦ Create an Executable Test Case (bundler/inline)が理想的 ◦

    事象が再現するソースコードレポジトリ + 再現手順が次善 • 再現手順(Steps to reproduce)ではないもの ◦ コードスニペット ▪ 誰かが不足した(明示されない)情報を補う必要がある ◦ スタックトレース ▪ さらに問題修正の難易度は上がる Issues : Steps to reproduce
  6. • あなたが望む動作を書く ◦ Issueとfeature request(新機能)の区別は難しいことがある ▪ 過去のバージョンでどう動いていたか ▪ https://api.rubyonrails.org/ に記載されている振る舞いか

    ◦ 新機能と判断されたらissueはクローズされる ▪ What about Feature Requests? ▪ rubyonrails-core - Ruby on Rails Discussions ◦ 新機能を実現するpull requestを書くこともできます Issues : Expected behavior
  7. • 再現ケースで起きる実際の動作を書く ◦ 実際の動作なので肯定文で書く ◦ 例: “It raises an ArgumentError…”など

    • 否定文だと必要な情報が追加されない ◦ 例: “It does not work.” “Not working” ◦ Expected behaviorではないこと以上がわからない Issues : Actual behavior
  8. • 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
  9. • Issueが発生するRubyのバージョン ◦ $ ruby -v の出力をかく ▪ 例: ruby

    3.3.5 (2024-09-03 revision ef084cc8f4) [x86_64-linux] ▪ “Ruby 3”ではどのバージョンかわからない ◦ 可能なら複数のRubyバージョンで再現するか、あるいは特定のバー ジョンでのみ再現する、しないなどの情報はなおよい Issues : System configuration Ruby version
  10. • Contributing to the Rails Code の内容を型として利用しましょう • Pull request

    template を型として利用しましょう • Pull Requestに書くこと ◦ コードとテスト、コミットメッセージ ◦ Title ◦ Motivation / Background ◦ Detail ◦ Additional information ◦ Checklist Pull requests
  11. • Pull request templateは型として利用しましょう ◦ 型がなくてもpull requestの必要性を説明できる場合は、必ずしも、 テンプレートに従う必要はありません ◦ No

    description provided. のpull requestには何もしません ◦ 私のpull requestはテンプレートに従っています ▪ コミットメッセージの後にテンプレートが来る ▪ 編集がもう少し楽になればいいのにと思うこともある Pull request template
  12. • Active Record ◦ ConnectionAdapters ◦ Migration ◦ データベースに特化した機能(PostgreSQL/MySQL/SQLite) •

    RailtiesやActive Support ◦ わかるところだけ 私がレビューしている pull requestの分野
  13. • 標準でSQLite, MySQL, PostgreSQLの3つのデータベースに対応 ◦ データベース共通機能と特化機能のバランス ◦ 特定のデータベースのみに存在する機能は入りにくい • データベースの機能を全てカバーすることは意図していない

    ◦ Railsのアプリケーションに必要な機能かどうか ◦ MigrationのDSLで表現できなくても、execute メソッドでRaw SQL が実行する手法がある ConnectionAdaptersの特徴
  14. • すぐにマージできるもの : 数は多い • マージに向けてレビューした方がいいと思うもの ◦ 主に内容のレビューに時間がかかるため、数は少ない • pull

    requestの方針に疑問、反対の意見を持っているもの ◦ 質問をしたり、反対の意見を述べたり、クローズしたりする ◦ 主に作文に時間がかかるため、数は少ない • どちらでもよいもの ◦ 何もしない ◦ 現実的にはこれが最も多い(限られたリソース) Pull requestをレビューするかの観点
  15. • いちばん重要な要素 ◦ Real world use case?と聞かれた人もいると思います ◦ あなたのユースケースであることが重要 ▪

    理由を説明できるから • ユースケースの種かも知れないが、ユースケースではまだないもの ◦ データベースに新機能が追加された ▪ 利用する全ての技術要素の対応を目的にしていないから ◦ 魅力的な技術の場合は、例外もありうる 1. ユースケースがあるか
  16. • https://github.com/rails/rails/pull/52285 • 実際のユースケースがあるかと質問したが、特にないとのこと ◦ ユースケースがある方は上記pull requestにコメントください • ユースケースなしでマージすると、それが将来の類似pull requestを

    マージする理由になってしまう • PostgreSQLバージョンによる分岐を追加したくなかった ◦ RailsはPostgreSQL 9.3以上に対応 ◦ 小さな変更だからマージすればいいのではというコメントもあったが、 コードが複雑する一歩である 例:PostgreSQL 10+でのmacaddr8データ型
  17. • 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作成
  18. • Railsは他のソフトウェアと共に用いられる ◦ 問題を解決すべきレイヤーで解決したい • 各ユーザーがをmonkey patchできることのメリット、デメリット ◦ メリット :

    ユーザーがquick fixできる ◦ デメリット : 副作用の発生、問題解決すべきレイヤーの見逃し • Railsが利用するソフトウェアの開発体制を知るのも良い経験 3.そのユースケースは Railsで解決すべきか
  19. • 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の”汚染”
  20. • マージに必要なアクションのアドバイスをもらう ◦ 第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の”汚染”
  21. • Railsのleaky abstraction(漏れのある抽象化) ◦ RailsConf 2018: Opening Keynote: FIXME by

    David Heinemeier Hansson • RailsのMigrationは、DDLが透けて見えるような実装 ◦ 複雑なSQLの全ての文法には対応できない ◦ SQL生成をするDSLの実装/APIの考慮は難しい 4.適切な抽象化をしているか
  22. • 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制約を遅延
  23. • 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制約を遅延
  24. • Issueやpull request へのコメントが :+1: で埋められる解消策 • レビュー時にあまり参考にしていない( +1 以上でも以下でもない)

    ◦ 数が多いからといってマージするわけではない ◦ 多すぎるEmojiリアクションは、”組織票”の可能性を考える ◦ 多くのユーザーにとって必要なら、ひろくの反応があるはず • あなたのユースケースがあるなら、それを文章にしてコメントした方が良い Emoji
  25. • Pull requestやIssueが読まれるために必要なスキル ◦ Title,Descriptionも自然言語のみ ◦ GitHubのFiles changedタブまで、ソースコードは出てこない • Grammar

    in Use(アメリカ英語) ◦ 助動詞の意図とかとても参考になりました • 2020年代の今、生成AIや機械翻訳は有用なツール ◦ 最終的に記述する英語の意味が自分でもわかる必要はある 英語
  26. • やりたいことがあったら、Contributing to Ruby on Rails を読んで、や れることをやって、pull requestをだしましょう •

    First time-contributor ラベルのあるpull requestは意識して見てい ます 将来のFirst contributorの皆さんへ
  27. End