From 4908d0877caaee2f93291e368f5e0a083edde3c0 Mon Sep 17 00:00:00 2001 From: Tejas Bubane Date: Thu, 6 Jun 2019 13:00:04 +0530 Subject: [PATCH] [Fix #51] Add ability to whitelist receiver class names In `Rails/DynamicFindBy`. Fixes https://github.com/rubocop-hq/rubocop-rails/issues/51. --- CHANGELOG.md | 2 ++ config/default.yml | 1 + lib/rubocop/cop/mixin/whitelist.rb | 28 +++++++++++++++++++ lib/rubocop/cop/rails/dynamic_find_by.rb | 10 +++---- lib/rubocop/cop/rails_cops.rb | 1 + .../rubocop/cop/rails/dynamic_find_by_spec.rb | 12 ++++++++ 6 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 lib/rubocop/cop/mixin/whitelist.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f396e83b5..d891408cd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ * Extract Rails cops from rubocop-hq/rubocop repository. ([@koic][]) * [#19](https://github.com/rubocop-hq/rubocop-rails/issues/19): Add new `Rails/HelperInstanceVariable` cop. ([@andyw8][]) +* [#51](https://github.com/rubocop-hq/rubocop-rails/issues/51): Add ability to whitelist receiver class names in `Rails/DynamicFindBy`. ([@tejasbubane][]) [@koic]: https://github.com/koic [@andyw8]: https://github.com/andyw8 +[@tejasbubane]: https://github.com/tejasbubane diff --git a/config/default.yml b/config/default.yml index 769a19dfc8..e40748ca00 100644 --- a/config/default.yml +++ b/config/default.yml @@ -141,6 +141,7 @@ Rails/DynamicFindBy: VersionAdded: '0.44' Whitelist: - find_by_sql + - Gem::Specification Rails/EnumUniqueness: Description: 'Avoid duplicate integers in hash-syntax `enum` declaration.' diff --git a/lib/rubocop/cop/mixin/whitelist.rb b/lib/rubocop/cop/mixin/whitelist.rb new file mode 100644 index 0000000000..2beb0d149a --- /dev/null +++ b/lib/rubocop/cop/mixin/whitelist.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Common functionality for checking whitelisted methods & class names. + module Whitelist + def whitelisted?(node) + whitelisted_method?(node) || whitelisted_receiver?(node) + end + + def whitelisted_method?(node) + whitelist.include?(node.method_name.to_s) + end + + def whitelisted_receiver?(node) + return unless node.receiver + + whitelist.include?(node.receiver.source.to_s) + end + + private + + def whitelist + cop_config['Whitelist'] + end + end + end +end diff --git a/lib/rubocop/cop/rails/dynamic_find_by.rb b/lib/rubocop/cop/rails/dynamic_find_by.rb index e81afe9614..8f4754e4ec 100644 --- a/lib/rubocop/cop/rails/dynamic_find_by.rb +++ b/lib/rubocop/cop/rails/dynamic_find_by.rb @@ -26,21 +26,21 @@ module Rails # # good # User.find_by!(email: email) class DynamicFindBy < Cop + include Whitelist + MSG = 'Use `%s` instead of dynamic `%s`.' METHOD_PATTERN = /^find_by_(.+?)(!)?$/.freeze def on_send(node) - method_name = node.method_name.to_s - - return if whitelist.include?(method_name) + return if whitelisted?(node) + method_name = node.method_name static_name = static_method_name(method_name) - return unless static_name add_offense(node, message: format(MSG, static_name: static_name, - method: node.method_name)) + method: method_name)) end alias on_csend on_send diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index ecf3854158..8157fe8381 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -9,6 +9,7 @@ module Cop end require_relative 'mixin/target_rails_version' +require_relative 'mixin/whitelist' require_relative 'rails/action_filter' require_relative 'rails/active_record_aliases' diff --git a/spec/rubocop/cop/rails/dynamic_find_by_spec.rb b/spec/rubocop/cop/rails/dynamic_find_by_spec.rb index 9d578966bb..29d19f7cf2 100644 --- a/spec/rubocop/cop/rails/dynamic_find_by_spec.rb +++ b/spec/rubocop/cop/rails/dynamic_find_by_spec.rb @@ -139,6 +139,18 @@ RUBY end + context 'with whitelisted classname' do + let(:cop_config) do + { 'Whitelist' => %w[find_by_sql Gem::Specification] } + end + + it 'accepts class methods in whitelist' do + expect_no_offenses(<<~RUBY) + Gem::Specification.find_by_name("backend").gem_dir + RUBY + end + end + context 'when using safe navigation operator' do context 'with dynamic find_by_*' do let(:source) { 'user&.find_by_name(name)' }