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

MemoizedInstanceVariableName Still complaining about leading underscores #6349

Closed
luke-hill opened this issue Oct 2, 2018 · 10 comments
Closed

Comments

@luke-hill
Copy link

Expected behavior

Not to report the cop (not present pre 0.58), fixed in 0.58.0

Actual behavior

rubocop 0.57.2 - 1 file inspected 0 failures
rubocop 0.58.2 - 1 file inspected 1 failure
rubocop 0.59.2 - 1 file inspected 1 failure

Steps to reproduce the problem

# This is fine
def foo
  @foo ||= :foo
end

# This isn't fine
def _foo
  @_foo ||= :foo
end
features/support/cop_test.rb:6:3: C: Naming/MemoizedInstanceVariableName: Memoized variable @_foo does not match method name _foo. Use @_foo instead.

RuboCop version('s)

0.57.2 (using Parser 2.5.1.2, running on ruby 2.2.10 x86_64-linux)
0.58.2 (using Parser 2.5.1.2, running on ruby 2.2.10 x86_64-linux)
0.59.2 (using Parser 2.5.1.2, running on ruby 2.2.10 x86_64-linux)
@luke-hill
Copy link
Author

Linked items #6095 #6125

@mikegee
Copy link
Contributor

mikegee commented Oct 2, 2018

The default configuration disallows leading underscores: https://rubocop.readthedocs.io/en/latest/cops_naming/#namingmemoizedinstancevariablename

It seems like you want: EnforcedStyleForLeadingUnderscores :optional

@Drenmi Drenmi closed this as completed Oct 3, 2018
@luke-hill
Copy link
Author

luke-hill commented Oct 3, 2018

Hi there @Drenmi think this was closed prematurely.

Those documents don't actual stipulate that my method is against rubocop's guidelines. just that other methods are valid. There isn't an instance where def _foo is given as a bad input.

Furthermore the error message is actual misleading. Because it's advising me to do something I've already done. To rename my method which was previously named _foo to _foo, which is obviously impossible...?

Could someone perhaps shed more light on this, unless I'm missing something obvious this is a legitimate bug.

@matthewrudy
Copy link

matthewrudy commented Oct 3, 2018

I think the problem is the error message

$ cat new.rb
# frozen_string_literal: true

def _foo
  @_foo ||= calculate_expensive_thing
end
$ rubocop --version
0.59.2

$ rubocop new.rb
Inspecting 1 file
C

Offenses:

new.rb:4:3: C: Naming/MemoizedInstanceVariableName:
Memoized variable @_foo does not match method name _foo.
Use @_foo instead.

  @_foo ||= calculate_expensive_thing
  ^^^^^

1 file inspected, 1 offense detected

The error message should be

Memoized variable @_foo does not match method name _foo. Use @foo instead.

(or maybe something that explains the bit about the underscoring)

@luke-hill
Copy link
Author

Can this be re-opened. As it's still locking us to rubocop 0.57.2 which is quite far behind.

cc/ @Drenmi / @mikegee

@luke-hill
Copy link
Author

Could someone maybe point me in the right direction here? As it's a reasonably large open source gem I'm running and this is the only dependency which is quite old.

@matthewrudy
Copy link

@luke-hill if you don't agree with it, maybe just turn off this cop.

# .rubocop.yml

Naming/MemoizedInstanceVariableName:
  Enabled: false

@mikegee
Copy link
Contributor

mikegee commented Dec 13, 2018

Could someone maybe point me in the right direction here?

Sure! Try removing this pending and try to make it pass.

@luke-hill
Copy link
Author

Right so it's actually something being worked on currently. That makes sense then.

@mikegee
Copy link
Contributor

mikegee commented Dec 13, 2018

I’m not sure anyone is currently working on it. You might be the most motivated for this particular task.

matthewrudy added a commit to matthewrudy/rubocop that referenced this issue Dec 19, 2018
…ut leading underscores

Rewrite the check for the variable name to bear in mind more potential cases.
matthewrudy added a commit to matthewrudy/rubocop that referenced this issue Dec 19, 2018
…thod name

The following code should be valid:

    def _foo
      @_foo ||= :foo
    end

But before this change it complains that the variable name should be @foo.

The rules need to take into account what the method name is,
and the configured style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants