mbj / devtools

The rake ci task!
MIT License
64 stars 20 forks source link

Add rubocop-rspec #87

Open backus opened 9 years ago

backus commented 9 years ago

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 commented 9 years ago

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.

mbj commented 9 years ago

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.

backus commented 8 years ago

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