Break cache if Gemfile.lock or .rubocop.yml change.
#300
Conversation
There was a problem hiding this comment.
@joshuapinter I know you didn't ask me for a review but thought I'd leave a few comments - the changes look pretty good so far! I would add some unit tests to cli_spec.rb or cache_spec.rb. Right now we're not covering busting the cache very well but we could be and test the scenarios that should bust it or at least the new ones. Nice work.
lib/erb_lint/cache.rb
Outdated
| @@ -76,7 +78,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}#{@rubocop_config}#{@gemfile_lock}#{file_content}" | |||
There was a problem hiding this comment.
With the amount of variables in the cache key now and no actual string content, should we break it out line by line to make it a little more clear vs the current string interpolation with something like:
irb(main):023:0> str_element_1 = "hello"
=> "hello"
irb(main):024:0> str_element_2 = "hi"
=> "hi"
irb(main):025:0> rubocop_config = "some_config"
=> "some_config"
irb(main):026:0] %w[
irb(main):027:0] str_element_1
irb(main):028:0] str_element_2
irb(main):029:0] rubocop_config
irb(main):030:0> ].join
=> "str_element_1str_element_2rubocop_config"
Just a style thing but might make this more readable especially if additional keys are added later.
| @@ -9,6 +9,8 @@ def initialize(config, cache_dir = nil) | |||
| @cache_dir = cache_dir || CACHE_DIRECTORY | |||
| @hits = [] | |||
| @new_results = [] | |||
| @gemfile_lock = File.read("Gemfile.lock") if File.exist?("Gemfile.lock") | |||
There was a problem hiding this comment.
Nice, I think this should effectively cache the File.read for all calls of checksum.
|
I didn’t ask but I was secretly hoping you would. Lol.
I’ll respond to your comments after the kids go to bed. Ha.
…On Jan 8, 2023 at 3:34 PM -0700, Zach Feldman ***@***.***>, wrote:
@zachfeldman commented on this pull request.
@joshuapinter I know you didn't ask me for a review but thought I'd leave a few comments - the changes look pretty good so far! I would add some unit tests to cli_spec.rb or cache_spec.rb. Right now we're not covering busting the cache very well but we could be and test the scenarios that should bust it or at least the new ones. Nice work.
In lib/erb_lint/cache.rb:
> @@ -76,7 +78,7 @@ def checksum(filename, file_content)
mode = File.stat(filename).mode
digester.update(
- "#{mode}#{config.to_hash}#{ERBLint::VERSION}#{file_content}"
+ ***@***.******@***.***_lock}#{file_content}"
With the amount of variables in the cache key now and no actual string content, should we break it out line by line to make it a little more clear vs the current string interpolation with something like:
irb(main):023:0> str_element_1 = "hello"
=> "hello"
irb(main):024:0> str_element_2 = "hi"
=> "hi"
irb(main):025:0> rubocop_config = "some_config"
=> "some_config"
irb(main):026:0] %w[
irb(main):027:0] str_element_1
irb(main):028:0] str_element_2
irb(main):029:0] rubocop_config
irb(main):030:0> ].join
=> "str_element_1str_element_2rubocop_config"
Just a style thing but might make this more readable especially if additional keys are added later.
In lib/erb_lint/cache.rb:
> @@ -9,6 +9,8 @@ def initialize(config, cache_dir = nil)
@cache_dir = cache_dir || CACHE_DIRECTORY
@hits = []
@new_results = []
+ @gemfile_lock = File.read("Gemfile.lock") if File.exist?("Gemfile.lock")
Nice, I think this should effectively cache the File.read for all calls of checksum.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Can I pick it up from here or are you going to get back around to it @joshuapinter? The new(ish?) cache feature brings my app’s linting w/ |
|
@mculp Please do! The only thing that should be needed are proper specs. We've been using this in development and in CI for the last couple months and it's been working perfectly, always breaking cache when gems or configs have been updated. Makes using it very quick most of the time but also very reliable when things change. |
joshuapinter
force-pushed
the
main
branch
2 times, most recently
from
ae61b11
to
2ca4dee
Compare
|
Hmmm, just upgraded our fork from 0.3.1 to 0.4.0 and the caching doesn't seem to be working. Every time I run I also tried using the main repo to see if it was related to this fork but seeing it there too. If I revert to 0.3.1 on either our fork or the main gem, the cache is properly used. Anybody else seeing this? |
|
@joshuapinter we've been running 0.4.0 for a while now and I haven't noticed the cache not working. I just tried locally and it appears to be working. I tried moving the |
|
@zachfeldman Thanks for the quick response! So strange... I removed the $ erblint
====
Running ERB Lint...
Cache mode is on
Linting 628 files with 9 linters...
Cache being created for the first time, skipping prune
No errors were found in ERB files
$ erblint
====
Running ERB Lint...
Cache mode is on
Linting 628 files with 9 linters...
Cache being created for the first time, skipping prune
No errors were found in ERB files
$ erblint
====
Running ERB Lint...
Cache mode is on
Linting 628 files with 9 linters...
Cache being created for the first time, skipping prune
No errors were found in ERB files |
Fixes #299.
Wrote in the addition of the
Gemfile.lockand the.rubocop.ymlfiles into the cache so that if either of those change, the cache will break.I noticed you were already doing this with
.erb-lint.ymlso that's great, no need to make any changes there. I just followed a similar pattern.I placed the reading of both of these files in the
Cache#initializemethod so it doesn't get run every timechecksumis called - maybe a little more performant.Besides that, I tested this out extensively in development by disabling the pruning and making various changes to see if the caches were hit or not. Everything seemed to work great.
I attempted writing specs but got a little lost with how to make it work correctly. I couldn't see any pattern laid down with how you handle when
.erb-lint.ymlconfig is changed so I left that for now. Open to comment and suggestion.We're gonna use our fork in "production" to test it more thoroughly and ensure any config or Gemfile changes won't produce false negatives.
TODOs