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

Faulty calculation in UncommunicativeName #8229

Closed
marcandre opened this issue Jul 1, 2020 · 2 comments · Fixed by #9357
Closed

Faulty calculation in UncommunicativeName #8229

marcandre opened this issue Jul 1, 2020 · 2 comments · Fixed by #9357
Labels
bug good first issue Easy task, suitable for newcomers to the project help wanted

Comments

@marcandre
Copy link
Contributor

marcandre commented Jul 1, 2020

I didn't check this cop closely, but just looking at the code, there are multiple errors in the calculation of the offending range.

I think the following examples will lead to the wrong selection range: any_offensive_name_with_underscore, *anyoffensivename, **anyoffensivename.

Code can probably simply rely on location.name (not sure, didn't check all dependents)

@marcandre marcandre added bug good first issue Easy task, suitable for newcomers to the project help wanted labels Jul 1, 2020
@harrylewis
Copy link

Hey @marcandre 👋 I'm interested in picking this up.

I just want to confirm the buggy behaviour first. I created a file that looks like the following.

# frozen_string_literal: true

def test(_x); end

Running rubocop on this file produces the following offense.

Inspecting 1 file
C

Offenses:

app.rb:5:10: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test(_x); end
         ^

1 file inspected, 1 offense detected

The expected behaviour should be that the calculated location is over all of _x, right?

Inspecting 1 file
C

Offenses:

app.rb:5:10: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test(_x); end
         ^^

1 file inspected, 1 offense detected

I see similar issues when using * and **.

@marcandre
Copy link
Contributor Author

@harrylewis: indeed.

ohbarye added a commit to ohbarye/rubocop that referenced this issue Jan 9, 2021
`UncommunicativeName#check` does faulty calculation of location when a
parameter has a prefix like `_`, `*`.

As for the file below,

```ruby
def test1(_a); end

def test2(__a); end

def test3(*a); end

def test4(**a); end

def test5(**__a); end
```

it shows offenses with faulty locations.

```shell
$ rubocop test.rb
Inspecting 1 file
C

Offenses:

test.rb:3:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test1(_a); end
          ^
test.rb:5:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test2(__a); end
          ^
test.rb:7:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test3(*a); end
          ^
test.rb:9:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test4(**a); end
          ^
test.rb:11:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test5(**__a); end
          ^

1 file inspected, 5 offenses detected
```

This commit fixed the behavior and make it show the correct locations.

```shell
$ rubocop test.rb
Inspecting 1 file
C

Offenses:

test.rb:3:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test1(_a); end
          ^^
test.rb:5:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test2(__a); end
          ^^^
test.rb:7:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test3(*a); end
          ^^
test.rb:9:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test4(**a); end
          ^^^
test.rb:11:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test5(**__a); end
          ^^^^^

1 file inspected, 5 offenses detected
```
bbatsov pushed a commit that referenced this issue Jan 9, 2021
`UncommunicativeName#check` does faulty calculation of location when a
parameter has a prefix like `_`, `*`.

As for the file below,

```ruby
def test1(_a); end

def test2(__a); end

def test3(*a); end

def test4(**a); end

def test5(**__a); end
```

it shows offenses with faulty locations.

```shell
$ rubocop test.rb
Inspecting 1 file
C

Offenses:

test.rb:3:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test1(_a); end
          ^
test.rb:5:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test2(__a); end
          ^
test.rb:7:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test3(*a); end
          ^
test.rb:9:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test4(**a); end
          ^
test.rb:11:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test5(**__a); end
          ^

1 file inspected, 5 offenses detected
```

This commit fixed the behavior and make it show the correct locations.

```shell
$ rubocop test.rb
Inspecting 1 file
C

Offenses:

test.rb:3:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test1(_a); end
          ^^
test.rb:5:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test2(__a); end
          ^^^
test.rb:7:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test3(*a); end
          ^^
test.rb:9:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test4(**a); end
          ^^^
test.rb:11:11: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def test5(**__a); end
          ^^^^^

1 file inspected, 5 offenses detected
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Easy task, suitable for newcomers to the project help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants