[Feature] Implement file cache #268

zachfeldman · 2022-08-15T00:10:27Z

Building on #191, this attempts to finish up the implementation of a file cache. Also adds associated tests for cli class and new tests for cache class.

ChrisBr

Looks great to me 👍

ChrisBr · 2022-08-15T06:02:45Z

lib/erb_lint/cache.rb

+      @file_loader = file_loader
+      @hits = []
+      @new_results = []
+      puts "Cache mode is on"


Not sure how this is handled in the rest of the library but maybe we should pass in an io or logger so we could silence these outputs?

Something like

def initialize(config, file_loader = nil, logger = STDOUT) # ... @logger = logger @logger.puts "Cache mode is on" end

That seems like a nice addition, the rest of the library though uses puts to output to $stdout though. I did try the code you recommended, but I'm not really sure if that allows an easy way to silence output (at least I couldn't Google it, and couldn't make it happen running the command without output unless I redirect to > /dev/null). Do you have a good example of this pattern in another lib? I could also go for a full blown logger instance with debug levels for most of the stuff in this file but then I feel like I'd want to apply that to all puts statements in the project.

If the rest of the library uses puts this is fine.

To silence it in tests you can pass in a StringIO like

io = StringIO.new Foo.new(io: io) io.string

ChrisBr · 2022-08-15T06:03:40Z

lib/erb_lint/cache.rb

+      mode = File.stat(file).mode
+
+      digester.update(
+        "#{file}#{mode}#{config.to_hash}"


I wonder if we should put the library version into the cache key too?

Good call. Added a commit to do that.

ChrisBr · 2022-08-15T06:04:42Z

lib/erb_lint/cli.rb

+          cache.clear
+          success!("cache directory cleared")
+        else
+          failure!("cache directory already doesn't exist, skipped deletion.")


This sentence doesn't sound like correct english?

Suggested change

failure!("cache directory already doesn't exist, skipped deletion.")

failure!("cache directory doesn't exist, skipped deletion.")

Yeah not sure how that ended up there, probably some copy pasta - fixed in the latest update.

ChrisBr · 2022-08-15T06:06:33Z

lib/erb_lint/cli.rb

+    def prune_cache?
+      @options[:prune_cache]
+    end


I wonder if we can also be a bit more smart here? Something what Rubocop does with max files in cache and then we prune the oldest when it's getting higher.

We definitely could but, naively, you can just run erblint <files> --with-cache --prune-cache every time and the code as written will remove outdated cache files only. Maybe we can leave more smart cache pruning for a future PR?

rafaelfranca · 2022-08-16T17:08:21Z

lib/erb_lint/cli.rb

      @files = @options[:stdin] || dupped_args

      load_config

+      @cache = Cache.new(@config, file_loader) if with_cache? || clear_cache?
+      if clear_cache?


Suggested change

if clear_cache?

if clear_cache?

Made my own commit to fix these newline suggestions!

rafaelfranca · 2022-08-16T17:09:28Z

lib/erb_lint/cli.rb

+
+    def run_on_file(runner, filename)
+      file_content = read_content(filename)
+      if with_cache? && !autocorrect?


Suggested change

if with_cache? && !autocorrect?

if with_cache? && !autocorrect?

rafaelfranca · 2022-08-16T17:09:35Z

lib/erb_lint/cli.rb

+      else
+        file_content = run_with_corrections(runner, filename, file_content)
+      end
+      log_offense_stats(runner, filename)


Suggested change

log_offense_stats(runner, filename)

log_offense_stats(runner, filename)

rafaelfranca · 2022-08-16T17:10:43Z

lib/erb_lint/cli.rb

+    end
+
+    def run_using_cache(runner, filename, file_content)
+      if cache.include?(filename) && !autocorrect?


This method is never called with autocorrect? so we don't need to check again.

Suggested change

if cache.include?(filename) && !autocorrect?

if cache.include?(filename)

Good catch! I removed it in the latest update.

rafaelfranca · 2022-08-16T17:13:10Z

lib/erb_lint/cache.rb

+      mode = File.stat(file).mode
+
+      digester.update(
+        "#{file}#{mode}#{config.to_hash}"


zachfeldman · 2022-08-16T17:54:57Z

Thanks for your comments @ChrisBr and @rafaelfranca ! I'm hoping to address them one night this evening or this weekend since I'm working on this PR in my spare time.

zachfeldman · 2022-08-19T00:36:04Z

@ChrisBr @rafaelfranca thanks for the comments, I think I've replied to all of them. Are either of you able to approve me to run the CI workflow against my branch?

zachfeldman · 2022-08-20T02:13:06Z

@rafaelfranca I think the tests should be fixed, I believe they were failing on CI because the checksum was slightly different there vs locally. I mocked out some of the methods it uses to generate the checksum instead and everything is passing for me locally still.

ChrisBr

Nice 👏

zachfeldman · 2022-08-26T14:01:33Z

@rafaelfranca or @etiennebarrie are you able to run CI for me? I have some time today to fix things up if there are still failures 😃

First approach to implement a file cache. Shopify#158 Co-authored-by: Mike Dalessio <mike.dalessio@gmail.com>

zachfeldman · 2022-08-30T00:49:38Z

Rebased against master, specs were passing before the rebase but just want to stay up to date.

ChrisBr · 2022-08-30T07:22:52Z

@rafaelfranca do you want to have another look and 🚢 ?

zachfeldman · 2022-09-06T14:24:29Z

Hey good morning ya'll! @etiennebarrie are you able to give this a review as another maintainer that seemingly has write access? I know @rafaelfranca can get pretty busy with lots of projects under his belt, so just exploring if there's another way to move this forward in the meantime. Thanks for your consideration!

Co-authored-by: Eric Terry <eterry1388@aol.com>

etiennebarrie

I'm not seeing the exceptions I reported last time.

We do have cops that error, and when you use the cache you don't get the error anymore, but I'm not sure we need to do something about it.

Tiny feedback about the name: I wish it was --cache rather than --with-cache. Also should we enable pruning by default? What's the reason for not pruning?

lib/erb_lint/cli.rb

…/implement-file-cache Support caching with json and compact reporters

Co-authored-by: Étienne Barrié <etienne.barrie@gmail.com>

zachfeldman · 2022-10-13T12:05:26Z

@etiennebarrie great! As mentioned before the cache is opt-in, so I'm not too concerned about that - subsequent pull requests can be made if people aren't happy with what information is stored/restored for their specific linter/use case, but I think this is far beyond a basic skeleton to add to for this feature at this point. I think I'm about ~25 hours of work in.

I changed the name to cache (this was a holdover from the first implementation https://github.com/Shopify/erb-lint/pull/191/files#diff-1156d85dde58090796995118a6420fe59feb9f48234b5f550f3b9ed2b2ea23a6R131). I also enabled pruning by default - I agree that it makes sense to do so, it just adds slightly more risk to those running the tool for the first time, but it'll probably cause even more problems later on to not have it enabled by default.

I also added a README. Do you have any other concerns I can address? Thanks for your time.

etiennebarrie · 2022-10-13T14:41:24Z

I also enabled pruning by default

For comparison, I checked what Rubocop does, it prunes not on the files that didn't have a hit but on the total number of cache files, and removes the oldest ones. But it must make it possible to use caching while linting a single file without pruning all the other cached results (locally in dev typically).

it just adds slightly more risk to those running the tool for the first time

How so?

Do you have any other concerns I can address?

One thing I noticed is that we use the ability to tweak the Rubocop cache directory. It's not so much of a concern, more like a new feature that can be added in another PR.
I don't think we should display "Skipping deletion of new cache result" or "Cleaning deleted cached file with checksum". It just adds to the output without much value.

Other than that I tested it on our app and it works well.

zachfeldman · 2022-10-17T14:48:10Z

Great thanks @etiennebarrie ! I haven't had time yet to get back to look at those suggestions like I thought I would this weekend (man, the autumn is really beautiful here in New York so it's hard to want to be on my laptop) but I might some night this week or next weekend.

I wasn't clear on if these last comments are enough for you to withhold your approval/merging of this to main, or if these are just nice to haves - I think we just need you specifically to approve this to merge it at this point, since we have 2 other approvals and you're a maintainer, right? Meanwhile maybe we can merge this so more people can test it and make some of your suggestions in follow up PRs since this one is already pretty huge (perhaps indicated by the fact that there's already been a PR merged to this PR :)) and has been open for over 2 months now? Unless there's anything else in it that makes you uncomfortable/worried about a merge.

etiennebarrie · 2022-10-18T08:16:57Z

I wasn't clear on if these last comments are enough for you to withhold your approval

I'm just isn't clear about the risk that pruning adds?

it just adds slightly more risk to those running the tool for the first time

How so?

And I don't like the output of all files pruned

I don't think we should display "Skipping deletion of new cache result" or "Cleaning deleted cached file with checksum". It just adds to the output without much value.

The rest can be tackled later IMO.

zachfeldman · 2022-10-18T12:18:45Z

Great @etiennebarrie , see e082855 to reduce the output of the pruning process. Ideally there'd be some "verbose" mode for erb-lint I could optionally hook that output into but again I'm just trying to get to a merge at this point.

Regarding the pruning - my point about risk was mostly a general one. All of this code is pretty new, anything we can do to reduce the risk of those using it is good IMHO. Sometimes it's better to just try to do less - ideally I'd want to put out the --cache and --prune options and let people experiment with them before running them both at the same time. But a few people have tried this branch and seemingly not reported any issues I'm not thinking of, and the code is well unit tested, so it's probably fine. As I keep pointing out all of this should be opt-in anyway and not affect anyone not using the cache.

Appreciate the time you've spent on the review so far, it's definitely made the branch better.

zachfeldman · 2022-10-25T21:20:05Z

Hi @etiennebarrie , just following up, do you think it would be possible to get this approved and merged sometime this week if I've addressed your remaining concerns sufficiently? Glad to discuss anything else on your mind or try to recruit more people to test this branch if you'd like more real-world examples of it working for people!

etiennebarrie · 2022-10-26T11:52:06Z

I think we're good! Thanks a lot. I think we'll want another PR to make the directory configurable and then we can ship a new version.

zachfeldman · 2022-10-26T14:35:31Z

You're welcome @etiennebarrie and again super thanks for your help picking this one up and reviewing it! Your suggestions were very helpful and really ensured a high-quality diff was merged. Also props to @ChrisBr and @flavorjones for the foundational work here, @rafaelfranca for your initial review, and @eterry1388 and @joshuapinter for testing out the code and even providing a patch along the way.

I'm not sure if I'll have time for the change mentioned to make the cache file directory configurable soon, but will comment here or on the issue about the cache if I'm able to get to it.

ChrisBr · 2022-10-26T14:42:55Z

The power of open source 😄 Good work @zachfeldman and I'm glad this made it into erb-lint finally.

zachfeldman marked this pull request as ready for review August 15, 2022 00:16

zachfeldman mentioned this pull request Aug 15, 2022

Implement file cache #191

Draft

3 tasks

ChrisBr reviewed Aug 15, 2022

View reviewed changes

rafaelfranca reviewed Aug 16, 2022

View reviewed changes

zachfeldman force-pushed the zfeldman/cbruckmayer/implement-file-cache branch from 7b7ff2b to bc2193e Compare August 20, 2022 02:11

zachfeldman changed the title ~~Implement file cache~~ [Feature] Implement file cache Aug 20, 2022

zachfeldman force-pushed the zfeldman/cbruckmayer/implement-file-cache branch 2 times, most recently from f49bda2 to fe68af5 Compare August 20, 2022 15:33

ChrisBr approved these changes Aug 21, 2022

View reviewed changes

zachfeldman requested a review from rafaelfranca August 29, 2022 15:26

ChrisBr and others added 12 commits August 29, 2022 20:48

Implement file cache

fbf26a2

First approach to implement a file cache. Shopify#158 Co-authored-by: Mike Dalessio <mike.dalessio@gmail.com>

Initial cleaning implementation

17ce887

Rename clean to prune, implement clearing, add autocorrect error message

b5aeb01

Lints and cops

95b5e22

Add integration specs

7032a73

Fix cache implementation, clean up code

6012ae8

Specs for cache and associated changes

ad47b98

Clean up

fbbcd2a

Include ERBLint version in cache key

a292ef0

Fix nonsensical error message

be11370

Spacing fixes and removing unneeded code

deeb4f8

Fix tests by mocking returned checksum

1a5db05

zachfeldman force-pushed the zfeldman/cbruckmayer/implement-file-cache branch from fe68af5 to 1a5db05 Compare August 30, 2022 00:49

zachfeldman and others added 2 commits October 11, 2022 21:32

Update lib/erb_lint/cached_offense.rb to avoid errors on nil severity

ce8d6da

Co-authored-by: Eric Terry <eterry1388@aol.com>

Support caching with json and compact reporters

acba4a9

eterry1388 mentioned this pull request Oct 12, 2022

Support caching with json and compact reporters zachfeldman/erb-lint#1

Merged

etiennebarrie reviewed Oct 13, 2022

View reviewed changes

lib/erb_lint/cli.rb Outdated Show resolved Hide resolved

zachfeldman and others added 5 commits October 13, 2022 07:39

Merge pull request #1 from eterry1388/eterry1388/zfeldman/cbruckmayer…

6d492e4

…/implement-file-cache Support caching with json and compact reporters

Update lib/erb_lint/cli.rb to use safe navigation operator

9ec34e3

Co-authored-by: Étienne Barrié <etienne.barrie@gmail.com>

Rename with-cache to cache

877c28c

Prune cache by default

7ea048c

Add README for cache

a32816d

zachfeldman requested review from eterry1388 and etiennebarrie and removed request for eterry1388 and etiennebarrie October 13, 2022 12:00

Reduce output of pruning process

e082855

etiennebarrie merged commit 3f14d38 into Shopify:main Oct 26, 2022

adrianna-chang-shopify mentioned this pull request Oct 26, 2022

Allow custom cache directory via --cache-dir option #282

Merged

shopify-shipit bot temporarily deployed to production November 1, 2022 16:01 Inactive

joshuapinter mentioned this pull request Dec 27, 2022

Cache ignores Rubocop version and rubocop.yml configuration. #299

Open

	failure!("cache directory already doesn't exist, skipped deletion.")
	failure!("cache directory doesn't exist, skipped deletion.")

	if with_cache? && !autocorrect?

	if with_cache? && !autocorrect?

	log_offense_stats(runner, filename)

	log_offense_stats(runner, filename)

	if cache.include?(filename) && !autocorrect?
	if cache.include?(filename)

[Feature] Implement file cache #268

[Feature] Implement file cache #268

Conversation

zachfeldman commented Aug 15, 2022

ChrisBr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachfeldman commented Aug 16, 2022

zachfeldman commented Aug 19, 2022

zachfeldman commented Aug 20, 2022

ChrisBr left a comment

Choose a reason for hiding this comment

zachfeldman commented Aug 26, 2022

zachfeldman commented Aug 30, 2022

ChrisBr commented Aug 30, 2022

zachfeldman commented Sep 6, 2022

etiennebarrie left a comment

Choose a reason for hiding this comment

zachfeldman commented Oct 13, 2022

etiennebarrie commented Oct 13, 2022

zachfeldman commented Oct 17, 2022 • edited Loading

etiennebarrie commented Oct 18, 2022

zachfeldman commented Oct 18, 2022 • edited Loading

zachfeldman commented Oct 25, 2022

etiennebarrie commented Oct 26, 2022

zachfeldman commented Oct 26, 2022

ChrisBr commented Oct 26, 2022

zachfeldman commented Oct 17, 2022 •

edited

Loading

zachfeldman commented Oct 18, 2022 •

edited

Loading