Optimize Base#relevant_rubocop_rspec_file? #967
Conversation
|
And of course this doesn‘t work with specs (spec/rubocop/cop/rspec/cop_spec.rb) that change the configuration. |
lib/rubocop/cop/rspec/cop.rb
Outdated
| Config.new | ||
| .for_all_cops | ||
| .fetch('RSpec', default_configuration) | ||
| .fetch('Patterns') |
There was a problem hiding this comment.
Isn't the configuration merged with the default configuration, can we assume that AllCops/RSpec/Patterns is always present?
There was a problem hiding this comment.
You are right. We can just use ConfigLoader.default_configuration.for_all_cops here.
There was a problem hiding this comment.
Still, I’m struggling to figure out how to make this work with the changing configurations in cop_spec.rb
There was a problem hiding this comment.
Just found this https://github.com/rubocop-hq/rubocop/blob/8842e13621faa82c2688e6d5e7cd891e0aa5873c/lib/rubocop/rspec/shared_contexts.rb#L76
Maybe can be of some help?
There was a problem hiding this comment.
This works just fine https://github.com/rubocop-hq/rubocop-rspec/pull/1007/files#diff-9a93f375888ae8523835d1127abf853cR215
We have to deep-dup it though https://github.com/rubocop-hq/rubocop-rspec/pull/1007/files#diff-48b259d4fb76048490c5ce8753a8ed33R4 to prevent leakage.
There was a problem hiding this comment.
Thanks, but… I tried, and I cannot make it work.
Would you like to give it a shot @pirj – you’re welcome to add a commit to my branch. (I’ve just rebased it, so make sure to re-fetch first).
There was a problem hiding this comment.
Yeah, I hoped this was a small enough change that we could merge it pre-2.0.
There was a problem hiding this comment.
But of course, if testing makes it a post-2.0 we can wait.
There was a problem hiding this comment.
I mean it could be part of 2.0, not even post-2.0. We have a bunch of pull requests specifically for 2.0.
The result of `rspec_pattern` is always the same (because it is based on the configuration file), so there is no need to re-calculate it every time it is called. I tried memozing on the Cop instance, but at least when running the specs, we instantiate Cop way too often. Memoizing on the class level reduces the number of calculations to once per `Cop` subclass we have. If we allowed using class variables (`@@rspec_pattern`) we could reduce the number of calculations to one. This commit reverts 995b4fe and adds memoization in a class instance variable.
bquorning
force-pushed
the
rspec_pattern
branch
from
ef74bbd
to
ab89845
Compare
bquorning
changed the title
|
Fixed in #1063. |
Another take on #965.
The result of
rspec_patternis always the same (because it is based on the configuration file), so there is no need to re-calculate it every time it is called.I tried memozing on the
CopBase instance, but at least when running the specs, we instantiateCopBase way too often. Memoizing on the class level reduces the number of calculations to once perBasesubclass we have. If we allowed using class variables (@@rspec_pattern) we could reduce the number of calculations to one.This commit reverts 995b4fe and adds memoization in a class instance variable.
Before submitting the PR make sure the following are checked:
master(if not - rebase it).CHANGELOG.mdif the new code introduces user-observable changes.bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).