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 offense detected by Style/MixinUsage #4885

Closed
InteNs opened this issue Oct 19, 2017 · 6 comments
Closed

false offense detected by Style/MixinUsage #4885

InteNs opened this issue Oct 19, 2017 · 6 comments
Labels

Comments

@InteNs
Copy link

InteNs commented Oct 19, 2017

Expected behavior

thing = Class.new do
  attr_accessor :value_text, :value_type
  include IsNameValueType
end

RSpec.describe IsNameValueType do
  subject { thing.new }
  # tests
end

expect Mixin usage to be correct

Actual behavior

Inspecting 4 files
.C..

Offenses:

spec/models/concerns/is_name_value_type_spec.rb:3:3: C: Style/MixinUsage: include is used at the top level. Use inside class or module.
  include IsNameValueType
  ^^^^^^^^^^^^^^^^^^^^^^^

4 files inspected, 1 offense detected

Steps to reproduce the problem

create the test as in the expected behaviour and run rubocop <filename>

RuboCop version

Include the output of rubocop -V. Here's an example:

➔ rubocop -V
0.51.0 (using Parser 2.4.0.0, running on ruby 2.4.1 x86_64-darwin16)
@pocke pocke added the bug label Oct 19, 2017
@dreid
Copy link

dreid commented Oct 19, 2017

It looks like this cop misreports for any case where the class is the first in some set of siblings.

For example:

class A
  include Mixin
end

class B
  include Mixin
end

Result:

❯ rubocop t.rb
Inspecting 1 file
C

Offenses:

t.rb:2:3: C: include is used at the top level. Use inside class or module.
  include Mixin
  ^^^^^^^^^^^^^

1 file inspected, 1 offense detected

Of note is that only the first class has this error reported.

❯ rubocop -V
0.51.0 (using Parser 2.4.0.0, running on ruby 2.3.4 x86_64-darwin16)

@benhutton
Copy link

I'm also getting false positives for this:

RSpec::Matchers.define :do_something do
  include Mixin
end

Again, strangely, only for the first occurrence in a file. I define three matchers in one file, all with the same mixin, and only get flagged for the first one.

.....

It is a bit unclear from the mixin code itself and the PR what the goal of the cop is. Is it really to prevent mixing into Object? Is it to prevent mixing into main? Is it to prevent mixing in outside of a class or module? Let's state very explicitly what the goal is, and then implement against it more narrowly!

@RemyMaucourt
Copy link

RemyMaucourt commented Oct 25, 2017

I have the same problem, and another info.

class A
  include module_A
  include namespace_1::module_B
  include namespace_1::module_C
  include module_D
end

class B
   include module_E
end

Rubocop will raise Style/MixinUsage only for module_A and module_D, not the namespaced ones.

$ rubocop -V
0.51.0 (using Parser 2.4.0.0, running on ruby 2.4.1 x86_64-linux)

@Drenmi
Copy link
Collaborator

Drenmi commented Oct 25, 2017

Indeed. It's a bit unclear what the implementation is trying to do, but it's clear that it's incorrect. 🙂

koic added a commit to koic/rubocop that referenced this issue Nov 4, 2017
Fixes rubocop#4885.

This change makes the following modification based on Issue's report.

## Using `include` inside block

The follwing does not register an offense.

```ruby
Class.new do
  include IsNameValueType
end
```

and

```ruby
RSpec::Matchers.define :do_something do
  include Mixin
end
```

## Multiple definition classes in one

The follwing does not register an offense.

```ruby
class C1
  include M
end

class C2
  include M
end
```

## Nested module

The follwing register an offense.

```ruby
include M1::M2::M3

class C
end
```
bbatsov pushed a commit that referenced this issue Nov 5, 2017
Fixes #4885.

This change makes the following modification based on Issue's report.

## Using `include` inside block

The follwing does not register an offense.

```ruby
Class.new do
  include IsNameValueType
end
```

and

```ruby
RSpec::Matchers.define :do_something do
  include Mixin
end
```

## Multiple definition classes in one

The follwing does not register an offense.

```ruby
class C1
  include M
end

class C2
  include M
end
```

## Nested module

The follwing register an offense.

```ruby
include M1::M2::M3

class C
end
```
@ZASMan
Copy link

ZASMan commented Dec 26, 2017

Is there any fix on this? It's still popping up for me.

@pocke
Copy link
Collaborator

pocke commented Dec 27, 2017

@ZASMan
Can you try updating rubocop to 0.52.0? It has been fixed in 0.52.0 already.
If you still have any bugs, please open a new issue.

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

7 participants