Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1525](https://github.com/rubocop/rubocop-rails/issues/1525): Change `Rails::HelperInstanceVariable` not to detect offenses for instance variables within classes. ([@viralpraxis][])
33 changes: 16 additions & 17 deletions lib/rubocop/cop/rails/helper_instance_variable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module Rails
# variable, consider moving the behavior elsewhere, for
# example to a model, decorator or presenter.
#
# Provided that a class inherits `ActionView::Helpers::FormBuilder`,
# Provided that an instance variable belongs to a class,
# an offense will not be registered.
#
# @example
Expand All @@ -28,38 +28,37 @@ module Rails
# end
#
# # good
# class MyFormBuilder < ActionView::Helpers::FormBuilder
# @template.do_something
# module ButtonHelper
# class Welcome
# def initialize(text:)
# @text = text
# end
# end
#
# def welcome(**)
# render Welcome.new(**)
# end
# end
#
class HelperInstanceVariable < Base
MSG = 'Do not use instance variables in helpers.'

def_node_matcher :form_builder_class?, <<~PATTERN
(const
(const
(const {nil? cbase} :ActionView) :Helpers) :FormBuilder)
PATTERN

def on_ivar(node)
return if inherit_form_builder?(node)
return if variable_belongs_to_class?(node)

add_offense(node)
end

def on_ivasgn(node)
return if node.parent.or_asgn_type? || inherit_form_builder?(node)
return if node.parent.or_asgn_type? || variable_belongs_to_class?(node)

add_offense(node.loc.name)
end

private

def inherit_form_builder?(node)
node.each_ancestor(:class) do |class_node|
return true if form_builder_class?(class_node.parent_class)
end

false
def variable_belongs_to_class?(node)
node.each_ancestor(:class).any?
end
end
end
Expand Down
35 changes: 8 additions & 27 deletions spec/rubocop/cop/rails/helper_instance_variable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,36 +35,17 @@ def foo
RUBY
end

it 'does not register an offense when a class which inherits `ActionView::Helpers::FormBuilder`' do
it 'does not register an offense when an instance variable is defined within a class' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add it as a new test instead of updating the existing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed that one since there's nothing specific about ActionView::Helpers::FormBuilder class anymore. Does it make sense to keep it?

expect_no_offenses(<<~RUBY)
class MyFormBuilder < ActionView::Helpers::FormBuilder
def do_something
@template
@template = do_something
module ButtonHelper
class Button
def initialize(text:)
@text = text
end
end
end
RUBY
end

it 'does not register an offense when a class which inherits `::ActionView::Helpers::FormBuilder`' do
expect_no_offenses(<<~RUBY)
class MyFormBuilder < ::ActionView::Helpers::FormBuilder
def do_something
@template
@template = do_something
end
end
RUBY
end

it 'registers an offense when using a class which does not inherit `ActionView::Helpers::FormBuilder`' do
expect_offense(<<~RUBY)
class Foo
def do_something
@template
^^^^^^^^^ Do not use instance variables in helpers.
@template = do_something
^^^^^^^^^ Do not use instance variables in helpers.
def button(**args)
render Button.new(**args)
end
end
RUBY
Expand Down