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

False positive in Style/TrivialAccessors with inline private #5641

Closed
fujimura opened this issue Mar 7, 2018 · 4 comments
Closed

False positive in Style/TrivialAccessors with inline private #5641

fujimura opened this issue Mar 7, 2018 · 4 comments
Labels

Comments

@fujimura
Copy link
Contributor

fujimura commented Mar 7, 2018

Expected behavior

Not an offense. In this case, private / protected clause needs symbol returned from def but attr_reader returns nil, which causes TypeError.

Actual behavior

# a.rb
class A
  private def foo
    @foo
  end
end
$ rubocop -a a.rb
Inspecting 1 file
C

Offenses:

a.rb:2:11: C: [Corrected] Style/TrivialAccessors: Use attr_reader to define trivial reader methods.
  private def foo
          ^^^

1 file inspected, 1 offense detected, 1 offense corrected
# Corrected a.rb
class A
  private attr_reader :foo
end
$ ruby a.rb
a.rb:2:in `private': nil is not a symbol nor a string (TypeError)
        from a.rb:2:in `<class:A>'
        from a.rb:1:in `<main>'

Steps to reproduce the problem

Described in actual behavour.

RuboCop version

$ rubocop -V
0.53.0 (using Parser 2.5.0.2, running on ruby 2.4.2 x86_64-darwin16)
@Drenmi
Copy link
Collaborator

Drenmi commented Mar 12, 2018

You can still define private readers using attr_reader, which is what RuboCop is suggesting:

private

attr_reader :foo

although not using the new inline syntax.

@pocke
Copy link
Collaborator

pocke commented Mar 14, 2018

I don't think it is a false positive for the same reason as the @Drenmi comment.
But the auto-correction behaviour is a bug. So we should disable the auto-correction on private.

BTW, maybe we can add an option to ignore this case, i.e. this cop ignores private def foo ... with this option. But I'm not sure the option is necessary as I think the private; attr_reader :foo is better style.

@pocke pocke added the bug label Mar 14, 2018
pocke added a commit to pocke/rubocop that referenced this issue Mar 15, 2018
…def` with `private`

See rubocop#5641

This cop should not auto-correct `private def foo() @foo end`. Because `def foo() @foo end` returns `:foo`, but `attr_reader :foo` returns `nil`.

The `autocorrect` method checks only the parent is a `send` type. It does not check method name. Because the cop also should handle methods other than `private`, `public`, `protected`. For example, [finalist](https://github.com/joker1007/finalist) gem defines `final` method (`final def foo; end`).

Note
===

This pull-request does not close the related issue since the issue still has an problem if the auto-correction is fixed.
I think we should discuss the cop should have or not an option to ignore `private def foo() @foo end` on the issue.
bbatsov pushed a commit that referenced this issue Mar 16, 2018
…th `private` (#5691)

See #5641

This cop should not auto-correct `private def foo() @foo end`. Because `def foo() @foo end` returns `:foo`, but `attr_reader :foo` returns `nil`.

The `autocorrect` method checks only the parent is a `send` type. It does not check method name. Because the cop also should handle methods other than `private`, `public`, `protected`. For example, [finalist](https://github.com/joker1007/finalist) gem defines `final` method (`final def foo; end`).

Note
===

This pull-request does not close the related issue since the issue still has an problem if the auto-correction is fixed.
I think we should discuss the cop should have or not an option to ignore `private def foo() @foo end` on the issue.
@fujimura
Copy link
Contributor Author

I don't think it is a false positive for the same reason as the @Drenmi comment.
But the auto-correction behaviour is a bug. So we should disable the auto-correction on private.

Agreed. Thanks for fixing this!

This was referenced Mar 21, 2018
This was referenced Mar 21, 2018
@Darhazer
Copy link
Member

Fixed in 0.54. Can we close this?

@Drenmi Drenmi closed this as completed Apr 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants