#1510 でのレビュー指摘内容の修正 #1541
#1510 でのレビュー指摘内容の修正 #1541
Conversation
refs #1510 refs #1521 (comment) ### 概要 検索可能なタイプかどうかの判定処理を可読性のためにメソッドに切り出していたが、 Symbol#in で済むためそちらを使うようにした
refs #1510 refs #1521 (comment) ### 概要 214e427 で case文 に変更していて単純比較で済むものはメソッドに切り出す必要がなくなっていた そのため、 all や questions を判定する when節 は単純にシンボルを書くように修正した
refs #1510 refs #1521 (comment) ### 概要 下記コミットで修正漏れがあったため、該当箇所を修正した title: model の名前の動的生成及び model への変換処理をリファクタリング revision: 931559b
refs #1510 refs #1521 (comment) ### 概要 各モデル毎に params_for_keyword_search 及び groupings を定義していたが、重複部分が多かったため共通部分は Searchable に定義して差分がある場合に各クラスでオーバーライドするよう修正した また、 キーワードにより検索するカラムは DSL で定義できるようにした ### 修正内容 #### 共通部分を Searchable に定義して差分がある場合に各クラスでオーバーライドするよう修正 params_for_keyword_search 、groupings を各モデルで定義していたが、重複部分がかなり多かったため重複部分は Searchable に移動した Comment のみ commentable_type が検索条件として必要になるため、オーバーライドして差分プログラミングで対応した #### キーワード検索で使用する条件をDSLで設定できるように修正 キーワード検索の際に キーワードで検索されるカラムを DSL で設定できるよう Serachable.target_column_of_keyword を追加した Serachable.target_column_of_keyword で定義されるメソッドを private化 する方法は Object#__send__ で Module#private を実行する方法と tap の際に Module$private を実行する方法を検討したが、 複数のプログラマーの意見、可読性を考慮して後者を選択した refs #1521 (comment) #### concerning を使用しないよう修正 bootcamp では他では concerning が使用されておらず統一感・可読性の観点から使用しないことに決定した refs #1521 (comment) refs #1521 (comment) refs #1521 (comment) refs #1521 (comment) refs #1521 (comment) refs #1521 (comment)
refs #1510 refs #1521 (comment) ### 概要 commentable_type_eq での絞込が機能しているかどうかを検証するテストを追加した ### 修正内容 #### commentable_type_eq が機能しているかどうかを検証するテストを追加 コメントを検索する際は条件として commentable_type_eq を設定するが、 既存ではそれが機能しているかを検証するテストがなかった そのため、下記の2件のテストを追加し commentable_type_eq が機能しているかを検証した - :all 以外の document_type を指定して検索した結果が、指定した document_type のコメントのみであるかの検証 - document_type に何も指定せずに検索した結果に、全ての document_type が含まれているかの検証 refs #1521 (comment)
refs #1510 refs #1521 (comment) refs #1521 (comment) ### 概要 既存では想定外の type が Searcher#result_for に渡ってきた場合は空配列を返していたが、そもそも想定外の type が渡されるのは異常事態であるため、例外を発生させるように修正した #### 補足 現状、修正行の例外は Unreachable であるが、下記の理由からそのままにしている - /searchables?document_type=comments で検索した際の挙動をそのまま裏仕様として残しておくため - 今後の修正で reachable になったときのための防御的プログラミング 想定外の type に対する本質的な対応は #1528 (comment) への対応で行う予定 refs #1528 (comment)
app/models/comment.rb
Outdated
| target_column_of_keyword :description | ||
|
|
||
| class << self | ||
| private def params_for_keyword_search(searched_values = {}) |
There was a problem hiding this comment.
private defみたいな書き方はどれくらいメジャーなんでしょう？僕は使ってないな〜。
なので、
private
def ...にするか、そもそもprivateキーワードなしにする（クラスメソッドについては可視性についてはそこまでこだわらない）、でもいい気がします。
There was a problem hiding this comment.
以下の理由から private def を使っていました:bow:
- https://github.com/pocke/ruboty-ruby-jp 等でこの書き方を見かけていた
- rubocop で警告が出ていなかった
- 特異クラスのスコープ内で定義されているのは params_for_keyword_search のみのため、インデントが深くなるより
private defで書いた方が読みやすいのでは、と考えた - この書き方が珍しい書き方なのか一般的に許容されているのかが自分の中でわからなくなってきたため、レビューで意見をもらおうとした
今回は @JunichiIto 、 @komagata さんの意見を受けて、グルーピングで privateメソッド化 する記法に修正しました:pray:(併せて .rubocop.yml にルールを追加しています)
c.f. 50d55e0
app/models/concerns/searchable.rb
Outdated
| @@ -8,8 +8,23 @@ def search_by_keywords(searched_values = {}) | |||
| ransack(**params_for_keyword_search(searched_values)).result | |||
| end | |||
|
|
|||
| def target_column_of_keyword(*key_names) | |||
| define_singleton_method(:_grouping_condition) { "#{key_names.map(&:to_s).join("_or_")}_cont_all".to_sym }.tap(&singleton_class.method(:private)) | |||
There was a problem hiding this comment.
横に長くて読みにくい＆tapがごちゃごちゃしてわかりづらいので、これぐらいでいいのでは。
define_singleton_method(:_grouping_condition) do
"#{key_names.map(&:to_s).join("_or_")}_cont_all".to_sym
endクラスメソッドの可視性にはそこまで神経質にならない、というか、Rubyという言語の言語設計があまりクラスメソッドの可視性を気にしていなさそうな気がする（＝簡便にprivateなクラスメソッドを定義できない）ので、僕は無理にクラスメソッドをprivateにしない人です。
There was a problem hiding this comment.
e808563 で修正しました:bow:
tap以降の削除のみ行い、do end への書き換えは行っていません(理由はコミットメッセージを参照)
|
全体的にですが、「こういう書き方もできる」という珍しい書き方などは明確なメリットがなければそのプロジェクトの他のコードのスタイルに合わせて欲しいです。 |
…ations を追加 refs #1541 (comment) refs #1541 (comment) ### 概要 inline での privateメソッド化 がプロジェクト的に許容されているかを確認するために `private def` の形で書いていた しかしレビューにおいて @komagata さん・@JunichiIto さん の両名から使わない方がいいという意見をもらったため、グルーピングで privateメソッド化 する記法に修正した また今後同一の議論が発生しないようにするため、併せて .rubocop.yml に Style/AccessModifierDeclarations を追加した refs https://www.rubydoc.info/gems/rubocop/0.69.0/RuboCop/Cop/Style/AccessModifierDeclarations
refs #1541 (comment) ### 概要 Searchable#target_column_of_keyword で動的に定義される _grouping_condition を privateメソッド化 するために define_singleton_method からメソッドチェーンで privateメソッド化 していた しかし、レビューで「横に長くて読みにくい＆tapがごちゃごちゃしてわかりづらい」という意見をいただいたため、 tap で privateメソッド化 していた箇所を削除した レビューではブロックが curly brace での one-line から do end での multi-line も提案されていたが、以下の理由からそのまま curry brace での記述にしている - curly brace での記述の場合、line length は 111文字 - Style/LineLength を設定している場合、 120文字 に設定しているプロジェクトが結構多いように感じる - c.f. rubocop/rubocop-jp#32 (comment) - RubyMine の Code Style でのデフォルト設定が 120文字
普段はそうするようにしているのですが、その書き方が「珍しい」のかどうかの尺度が自分の中で修正途中でよくわからなくなってきていたため、レビューで意見を求めてみようということでの今回の PR でした:bow:
c.f. #1541 (comment) @komagata さんのコメントの対象として #1541 (comment) での指摘箇所が含まれているかはわからないのですが、こちらはクラスメソッドを動的に定義していてかつ include先 のクラスの特異クラスで privateメソッド化 する必要があったという都合により指摘前のような記述になっていました:pray: |
app/models/concerns/searchable.rb
Outdated
| @@ -8,8 +8,23 @@ def search_by_keywords(searched_values = {}) | |||
| ransack(**params_for_keyword_search(searched_values)).result | |||
| end | |||
|
|
|||
| def target_column_of_keyword(*key_names) | |||
There was a problem hiding this comment.
def columns_for_keyword_search(*columns)みたいなインターフェースにすると少し自然かな、と思ったんだけど、どうでしょう？
There was a problem hiding this comment.
そうですよねぇ😇
ただ今回は keyword_search のための column として commentable_type も含まれる場合もあり、 columns_for_keyword_search だと commentable_type もここで設定するようにも見えるインターフェースになるかな、と考えて keyword の対象となる column という意味合いで target_column_of_keyword としてみたんですがどうでしょう？:thinking:
There was a problem hiding this comment.
口頭で話したことのメモ
- どっちの名前でも
commentable_typeを設定するように見える/見えないには影響しないのでは？そもそも作った本人以外はcommentable_typeがどうこう、ということに興味がなさそう（存在にも気づかなさそう） - 伊藤の気になる点
- columnsのように複数形にした方が自然
- keywordだけでは何のキーワードがわからないので、「キーワード検索（keyword search）」で使われることを明示したい
- targetはカットしても影響はなさそう（そもそもターゲットになるからカラムを指定しているはず）
- ofが入るのはちょっと仰々しい
There was a problem hiding this comment.
389dbbe で修正しました 🙏
app/models/concerns/searchable.rb
Outdated
| @@ -8,8 +8,23 @@ def search_by_keywords(searched_values = {}) | |||
| ransack(**params_for_keyword_search(searched_values)).result | |||
| end | |||
|
|
|||
| def target_column_of_keyword(*key_names) | |||
| define_singleton_method(:_grouping_condition) { "#{key_names.map(&:to_s).join("_or_")}_cont_all".to_sym } | |||
There was a problem hiding this comment.
{ "#{key_names.join("_or_")}_cont_all" }だけでも動いたりしない？
There was a problem hiding this comment.
動くんですが、返り値としては Symbol を返すようにしたいというお気持ちです😀
どうでしょう？:thinking:
join の前の to_s に関して
なるほど〜、 Array#join は文字列でない要素に対しては to_str もしくは to_s するんですね
知見ありがとうございます〜😆
https://docs.ruby-lang.org/ja/latest/method/Array/i/join.html
こちらはたしかに消してしまってよさそうですね😄
最後の to_sym に関して
こちらはそのまま残しておきたいと思っています
パラメータの key が String でも Symbol でも 動くのは Ransack が内部で Hash#with_indifferent_access を使用しているから(c.f. ransack の実装コード)なのですが、この挙動に依存して ransack に渡すパラメータの key の型として String も Symbol も渡してしまうのはあまり良くない気がしています😌
e.g. 今後、 ransack 内部で Hash#with_indifferent_access ではなく通常の Hash をそのまま渡すようになった場合に動かなくなる可能性がある
そのため、防御的ではありますが key として必ず Symbol を設定する ようにするため、 _grouping_condition の最後に to_sym する実装は残しておきたいと思っています:thinking:
補足
指摘をもらってから groupings 内で to_sym する案も考えたんですが、こちらの案だと呼び出し側に to_sym を呼ぶ必要がある という知識が必要になってきてしまうので却下しました
There was a problem hiding this comment.
口頭で話したことのメモ
- テストを書いているのでransackの内部実装が変わっても気づけるはず
- ときには防御的になることも大事だが、不特定多数の開発者に使われるライブラリというわけでもない（安全性とシンプルさを天秤にかけたときに、安全性を選ぶメリットがあまり大きくない）ので、ここは防御的になるよりもYAGNIの精神でよりシンプルな実装を選択した方がベターなのでは
There was a problem hiding this comment.
ed957df で修正しました 🙏
refs #1510
refs #1521
概要
#1521 でのレビューで指摘された箇所を修正した
#1521 にそのままコミットを積むとコミット数が多くなるため、別PR に切り出した
修正内容