Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails/DynamicFindBy の誤検出 #54

Closed
znz opened this issue Jun 12, 2019 · 3 comments · Fixed by rubocop/rubocop-rails#466
Closed

Rails/DynamicFindBy の誤検出 #54

znz opened this issue Jun 12, 2019 · 3 comments · Fixed by rubocop/rubocop-rails#466

Comments

@znz
Copy link

znz commented Jun 12, 2019

rubocop-rails 2.0.1 の Rails/DynamicFindBy で

が誤検出されました。

とりあえず Exclude でファイルごと除外していますが、 tokens gem と似た感じで独自定義の find_by_なんとか も引っかかっていたので、許可するメソッド名を指定したいです。

find_by_id の方は ActiveRecord の方は指摘して欲しいので、 capybara を使った spec の方を別の書き方にした方が良いのかもしれないと思いました。

@koic
Copy link
Member

koic commented Apr 21, 2020

Tokens の find_by_valid_token の方は .rubocop.yml に以下の指定をすることで解決しそうです。

Rails/DynamicFindBy:
  Whitelist:
    - find_by_valid_token

https://github.com/rubocop-hq/rubocop-rails/blob/v2.5.2/config/default.yml#L163-L169

なお Rails/DynamicFindByWhitelist は、次の RuboCop Rails のリリース (おそらく 2.6.0) に含まれる予定の rubocop/rubocop-rails#69 で非推奨となり、 AllowedMethods というパラメータ名が推奨になる予定です。

Capybara の find_by_id については、偽陰性があるかもしれませんがレシーバーを指定しない形での find_by_id は検出しないといったデフォルトで有効のオプショナルを用意することで回避できる気がしました。実装について考えてみます。現状のワークアラウンドとしては Rails/DynamicFindBy への Exclude パラメータで回避したいテストのディレクトリパスを指定するといった方法が考えられます。

koic added a commit to koic/rubocop-rails that referenced this issue Apr 26, 2021
Fixes rubocop/rubocop-jp#54 (Ja)

This PR fixes a false positive for `Rails/DynamicFindBy`
when not inheriting `ApplicationRecord` and without no receiver.

A dynamic matcher without receiver is only possible if it inherits
`ApplicationRecord` (`ActiveRecord::Base`).

For example, Capybara's `find_by_id` and Token's `find_by_valid_token`
should be accepted by default.

- https://www.rubydoc.info/github/jnicklas/capybara/Capybara/Node/Finders#find_by_id-instance_method
- https://github.com/fnando/tokens
@koic
Copy link
Member

koic commented Apr 27, 2021

Capybara の find_by_id については、偽陰性があるかもしれませんがレシーバーを指定しない形での find_by_id は検出しないといったデフォルトで有効のオプショナルを用意することで回避できる気がしました。実装について考えてみます。

rubocop/rubocop-rails#466 にて、レシーバーなしの find_by_xxx 呼び出しは、ApplicationRecord (ActiveRecord::Base) を継承したクラスの中で使っている場合を除いて受け入れるようにしました。次にリリースする RuboCop Rails 2.10.0 で適用する予定です。リリースまでお待ちください。

@koic
Copy link
Member

koic commented May 5, 2021

Rails/DynamicFindBy cop についての調整を含んだ、RuboCop Rails 2.10.0 をリリースしました。
https://github.com/rubocop/rubocop-rails/releases/tag/v2.10.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants