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

Style/MixinUsage false positive in specs #5059

Closed
tagliala opened this issue Nov 14, 2017 · 4 comments
Closed

Style/MixinUsage false positive in specs #5059

tagliala opened this issue Nov 14, 2017 · 4 comments

Comments

@tagliala
Copy link
Contributor

tagliala commented Nov 14, 2017

The Style/MixinUsage finds false positive inside specs on the current master branch of RuboCop


Expected behavior

Does not report a lint

Actual behavior

Reports a lint

Steps to reproduce the problem

my_class_spec.rb

# frozen_string_literal: true

require 'spec_helper'

# Top level docs
class MyClass
  def self.descendants
    [String]
  end
end

describe MyClass do
  it 'includes class' do
    described_class.descendants.should include String
  end
end
$ rubocop my_class_spec.rb 
Inspecting 1 file
C

Offenses:

my_class_spec.rb:14:40: C: include is used at the top level. Use inside class or module.
    described_class.descendants.should include String
                                       ^^^^^^^^^^^^^^

1 file inspected, 1 offense detected

Real world use case: https://github.com/brigade/scss-lint/blob/master/spec/scss_lint/reporter_spec.rb

RuboCop version

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

$ rubocop -V
0.51.0 (using Parser 2.4.0.2, running on ruby 2.4.2 x86_64-darwin16)
@koic
Copy link
Member

koic commented Nov 15, 2017

Thanks for opening issue. I can reproduce it.

I think that this minimum case will be below.

do_something(include(M))

The difficulty is that it can not be judged by receiver object whether it is Module#include or other include methods.

However, as indicated in the issue example, there are some cases where it becomes false positive with RSpec.
On the other hand, I think that there are not many use cases to use Module#include as method arguments.

So it may be a practical solution to make do_something(include(M)) case false negative.

@Drenmi
Copy link
Collaborator

Drenmi commented Nov 15, 2017

I agree @koic. AFAIK, if the include call is a method argument, it's safe to assume it's not a mixin.

This would also mean the fix should be trivial:

return if include_node.argument?

@koic
Copy link
Member

koic commented Nov 15, 2017

@Drenmi Thanks for comment and advice. I will open a PR to resolve this issue.

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

This PR solves the issue based on false positives detected against
RSpec `Include matcher`.
https://relishapp.com/rspec/rspec-expectations/v/2-0/docs/matchers/include-matcher

If an `include` method is called with a method argument, treat it as false negative.
In this case, it's safe to assume it's not a mixin.
bbatsov pushed a commit that referenced this issue Nov 15, 2017
Fixes #5059.

This PR solves the issue based on false positives detected against
RSpec `Include matcher`.
https://relishapp.com/rspec/rspec-expectations/v/2-0/docs/matchers/include-matcher

If an `include` method is called with a method argument, treat it as false negative.
In this case, it's safe to assume it's not a mixin.
@tagliala
Copy link
Contributor Author

Thanks!

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

No branches or pull requests

3 participants