devtools icon indicating copy to clipboard operation
devtools copied to clipboard

Add rubocop-rspec

Open backus opened this issue 10 years ago • 3 comments

I mentioned this in the mutant slack chat a few days back. I'll lookup and paste the relevant discussion in a comment on this issue. At some point I would like to look into the following regarding rubocop-rspec:

1. How much time would adding rubocop-rspec add to rubocop's average runtime and therefore devtools runtime as a whole

Intuitively, it doesn't seem like the 8 cops that rubocop-rspec provides would significantly impact the runtime on top of the ~240 cops rubocop provides. With that said, @mbj has said before he doesn't want to slow down devtools much more until the everything is parallelized, so benchmarking the impact of adding rubocop-rspec is probably still important.

2. What the false positive rate is for each cops that it adds

Some of the cops might yield false positives regularly enough that the cops end up not being worth it. If only a few of the cops end up being worth it when accounting for false positives then the gem probably shouldn't be added at all.

backus avatar Sep 10 '15 21:09 backus

Relevant discussion:

backus: Would be an interesting addition to devtools https://github.com/nevir/rubocop-rspec

GitHub nevir/rubocop-rspec rubocop-rspec - Code style checking for RSpec files

mbj: @backus: We already run rubocop on ​**/*.rb​. ​*_spec.rb​ files inside ​spec​ are included. mbj: @backus: Unless ​rubocop-rspec​ brings rspec specific cops there is no win. backus: It does backus: Things like making sure

module Foo
  RSpec.describe(Bar)
end

is the file ​foo/bar_spec.rbmbj: I do not agree at all to this design. mbj: Does it enforce the ​module Foo​ part? backus: no I think it enforces one of a few options backus: https://github.com/nevir/rubocop-rspec/blob/master/lib/rubocop/cop/rspec/file_path.rb#L6-L16 backus: other cops: https://github.com/nevir/rubocop-rspec/tree/master/lib/rubocop/cop/rspec mbj: k, if its configuralbe it makes sense to have it. mbj: I wounder if we need parallelism to reduce the amount of sequential work in devtools. backus: other nice things like enforcing ​described_class​ over duplication when possible mbj: I see how rubocop-rspec plugs into rubocop mbj: But mid-term devtools will not scale anymore. mbj: @backus: I'm happy to accept PR / issues for this addition. mbj: The default config in the PR should make sure it fits the conventions we have on the existing tools like mutant. mbj: (likely there are some violations in mutant itself, that need to be fixed) mbj: Also there are some cases the "dump" tool will return false positives, so each cop must be weighted for false positive rate. mbj: Example is that the tool will likely not implement a scope resolution, so it might flag something as "should be replaced with ​decribed_class​ where this will not work here". (edited) mbj: @backus: This one for example will likely be a false positive: https://github.com/mbj/mutant/commit/3874b3f11a5e77e60a34300362de36600438b8f8#diff-2769816d16e2f82a90d1d97c1b1b0c7eR5 (edited) mbj: ​described_class​ is not available inside the block, curious if ​rspec-rubocop​ is sensitive to that scoping. mbj: Static scope analysis (apart lvars) is close to impossible in ruby.

backus avatar Sep 10 '15 22:09 backus

Intuitively, it doesn't seem like the 8 cops that rubocop-rspec provides would significantly impact the runtime on top of the ~240 cops rubocop provides. With that said, @mbj has said before he doesn't want to slow down devtools much more until the everything is parallelized, so benchmarking the impact of adding rubocop-rspec is probably still important.

I think that 8 cops are not that relevant. Adding rubocop-rspec is more an addition to the existing tool rubocop.

mbj avatar Sep 11 '15 09:09 mbj

Quick update: the DescribedClass cop now should not descend into any scope changing blocks as mentioned above.

backus avatar Aug 03 '16 19:08 backus