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

Avoid instance variables in helpers #29

Merged
merged 7 commits into from
Jan 7, 2019
Merged
Show file tree
Hide file tree
Changes from 4 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
4 changes: 4 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ Rails/HasManyOrHasOneDependent:
Include:
- app/models/**/*.rb

Rails/HelperInstanceVariable:
Include:
- app/helpers/**/*.rb

Rails/HttpStatus:
EnforcedStyle: symbolic
SupportedStyles:
Expand Down
4 changes: 4 additions & 0 deletions config/enabled.yml
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ Rails/HasManyOrHasOneDependent:
StyleGuide: 'https://github.com/rubocop-hq/rails-style-guide#has_many-has_one-dependent-option'
Enabled: true

Rails/HelperInstanceVariable:
Description: 'Do not use instance variables in helpers'
Enabled: true

Rails/HttpPositionalArguments:
Description: 'Use keyword arguments instead of positional arguments in http method calls.'
Enabled: true
Expand Down
39 changes: 39 additions & 0 deletions lib/rubocop/cop/rails/helper_instance_variable.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# This cop checks for use of the helper methods which reference
# instance variables.
#
# Relying on instance variables makes it difficult to re-use helper
# methods.
#
# If it seems awkward to explicitly pass in each dependent
# variable, consider moving the behaviour elsewhere, for
# example to a model, decorator or presenter.
#
# @example
# # bad
# def welcome_message
# "Hello " + @user.name
# end
#
# # good
# def welcome_message(user)
# "Hello " + user.name
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I would recommend string interpolation instead of concatenation, but that might make the example less clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d912a64

Copy link
Contributor

Choose a reason for hiding this comment

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

The specs were changed, but the comment/documentation was not. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bquorning Sorry, missed that. 2c00a6b.

# end
class HelperInstanceVariable < Cop
MSG = 'Do not use instance variables in helpers.'.freeze

def on_ivar(node)
add_offense(node)
end

def on_ivasgn(node)
add_offense(node)
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ module Cop
require_relative 'rails/find_each'
require_relative 'rails/has_and_belongs_to_many'
require_relative 'rails/has_many_or_has_one_dependent'
require_relative 'rails/helper_instance_variable'
require_relative 'rails/http_positional_arguments'
require_relative 'rails/http_status'
require_relative 'rails/inverse_of'
Expand Down
36 changes: 36 additions & 0 deletions manual/cops_rails.md
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,42 @@ Include | `app/models/**/*.rb` | Array

* [https://github.com/rubocop-hq/rails-style-guide#has_many-has_one-dependent-option](https://github.com/rubocop-hq/rails-style-guide#has_many-has_one-dependent-option)

## Rails/HelperInstanceVariable

Enabled by default | Supports autocorrection
--- | ---
Enabled | No

This cop checks for use of the helper methods which reference
instance variables.

Relying on instance variables makes it difficult to re-use helper
methods.

If it seems awkward to explicitly pass in each dependent
variable, consider moving the behaviour elsewhere, for
example to a model, decorator or presenter.

### Examples

```ruby
# bad
def welcome_message
"Hello " + @user.name
end

# good
def welcome_message(user)
"Hello " + user.name
end
```

### Configurable attributes

Name | Default value | Configurable values
--- | --- | ---
Include | `app/helpers/**/*.rb` | Array

## Rails/HttpPositionalArguments

Enabled by default | Supports autocorrection
Expand Down
31 changes: 31 additions & 0 deletions spec/rubocop/cop/rails/helper_instance_variable_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::HelperInstanceVariable do
subject(:cop) { described_class.new }

it 'reports uses of instance variables' do
expect_offense(<<-RUBY.strip_indent)
def welcome_message
"Hello" + @user.name
^^^^^ Do not use instance variables in helpers.
end
RUBY
end

it 'reports instance variable assignments' do
expect_offense(<<-RUBY.strip_indent)
def welcome_message(user)
@user_name = user.name
^^^^^^^^^^^^^^^^^^^^^^ Do not use instance variables in helpers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: would it be more consistent, and slightly more clear, if the highlight was limited to just the instance variable being assigned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give me some pointers on how to do that? The docs for add_offense seem a little sparse.

https://www.rubydoc.info/gems/rubocop/RuboCop%2FCop%2FCop:add_offense

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure! 🙂

First, use the ruby-parse utility provided with the parser gem, to find the locations available for this source map:

$ ruby-parse -L -e '@user_name = user.name'

You should see something like this:

@user_name = user.name
~~~~~~~~~~ name      
           ~ operator            
~~~~~~~~~~~~~~~~~~~~~~ expression

So in this case we want to target the name location. The #add_offense method takes an optional location argument that is automatically turned into a range. (Defaults to :expression.)

So to add an offense on the relevant part:

add_offense(node, location: :name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for that. Fixed in 6dd2d00.

end
RUBY
end

specify do
expect_no_offenses(<<-RUBY.strip_indent)
def welcome_message(user)
"Hello " + user.name
end
RUBY
end
end