Avoid instance variables in helpers #29
Conversation
| # This cop checks for use of the helper methods which reference | ||
| # instance variables. | ||
| # | ||
| # Relying on the existing of a particular instance variable |
There was a problem hiding this comment.
There are others reasons, but I'm still trying to find a good way to write out about.
There was a problem hiding this comment.
The wording is a bit off here. Consider:
Relying on instance variables makes it difficult to re-use helper methods.
The next part about moving the behavior out of the helper sounds good to me. 👍
There was a problem hiding this comment.
Thanks, fixed in f78531e.
|
Does |
| # | ||
| # # good | ||
| # def welcome_message(user) | ||
| # "Hello " + user.name |
There was a problem hiding this comment.
Nitpick: I would recommend string interpolation instead of concatenation, but that might make the example less clear.
There was a problem hiding this comment.
The specs were changed, but the comment/documentation was not. Is that intentional?
There was a problem hiding this comment.
@bquorning Sorry, missed that. 2c00a6b.
| expect_offense(<<-RUBY.strip_indent) | ||
| def welcome_message(user) | ||
| @user_name = user.name | ||
| ^^^^^^^^^^^^^^^^^^^^^^ Do not use instance variables in helpers. |
There was a problem hiding this comment.
Minor nit: would it be more consistent, and slightly more clear, if the highlight was limited to just the instance variable being assigned?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)There was a problem hiding this comment.
Thanks for that. Fixed in 6dd2d00.
andyw8
changed the title
|
Think this is ready for merging, @koic. 🙂 |
|
I think this cop is reasonable. Thanks! |
`Rails/HelperInstanceVariable` cop added by #29 isn't backported to RuboCop core. 2.0 has been proposed as the release version of RuboCop Rails, it did so. toshimaru/rubocop-rails#31 (comment) Cops importing from the RuboCop core has lower than 1.0 so I think it works.
`Rails/HelperInstanceVariable` cop added by #29 isn't backported to RuboCop core. 2.0 has been proposed as the release version of RuboCop Rails, it did so. toshimaru/rubocop-rails#31 (comment) Cops importing from the RuboCop core has lower than 1.0 so I think it works. And this commit updates `rake yard_for_generate_documentation` task to add metadata and adds document regenerated by `rake yard_for_generate_documentation`.
Fixes #19