Bust erblilnt cache on rubocop config changes via linter checksum #373
Conversation
|
I have signed the CLA! |
|
@oehlschl not at a computer right now but as the person who finished up the initial caching PR this looks directionally right to me. I wouldn't consider this a review but I think writing specs at this point would be worth it. You're probably right that some methods are due for a refactor but maybe a separate PR makes sense for that. |
| @@ -76,7 +80,7 @@ def checksum(filename, file_content) | |||
| mode = File.stat(filename).mode | |||
|
|
|||
| digester.update( | |||
| "#{mode}#{config.to_hash}#{ERBLint::VERSION}#{file_content}", | |||
| "#{mode}#{config.to_hash}#{ERBLint::VERSION}#{@runner_checksum}#{file_content}", | |||
There was a problem hiding this comment.
Note that this will bust all caches on release since Runner#checksum will return Digest::SHA1.new.hexdigest even if there are no linters with checksum methods. That method can be refactored to return nil in the default / empty case if desired.
There was a problem hiding this comment.
I like this idea better than #300, it makes sense that each linter could have its own cache invalidation, like the rubocop linter shows.
Let's add some specs!
| @@ -67,6 +67,10 @@ def clear | |||
| FileUtils.rm_r(@cache_dir) | |||
| end | |||
|
|
|||
| def set_runner_checksum(checksum) | |||
There was a problem hiding this comment.
| def set_runner_checksum(checksum) | |
| def runner_checksum=(checksum) |
There was a problem hiding this comment.
Sounds good, thanks for the feedback @etiennebarrie . Will do.
| @@ -63,6 +63,15 @@ def autocorrect(processed_source, offense) | |||
| end | |||
| end | |||
|
|
|||
| def checksum | |||
There was a problem hiding this comment.
Let's call this something more intention-revealing. Rails calls something similar cache_key, that might work.
Fixes #299.
Alternative implementation of #300, as discussed here: #300 (comment)
This PR introduces a new
checksumproperty to Linters, exposed in aggregate through the Runner, to enable busting the cache when linter config and dependencies not represented in the gloabl erb_lint config change.Specifically, it adds logic to the Rubocop linter to handle both explicit config changes in the rubocop config (.rubocop.yml, or however that config is sourced) and implicit config changes in the versions of rubcop and any rubocop plugin gems (via Gemfile.lock).
I tested this locally to confirm that:
I haven't yet added specs, since I wanted to submit this for directional feedback before further investment. I'm happy to change method names, patterns, etc., and I also wrote this PR to be additive and minimally invasive. (Other options would have included more refactor of the Cache class, likely to add the checksum as an initializer arg and defer instantiation until after the Runner was set up; I'm happy to make those changes as well, but I was not sure how much appetite or concern there was for changing interfaces in this lib.)