Repository navigation
Conversation
`checksum` doesn't say what the value is for. As suggested in the review on Shopify#373, linters and the runner expose `cache_key` instead, and `Cache#set_runner_checksum` becomes a `runner_cache_key=` writer. Cover each step from a linter's key to a cache miss: linters have no key by default, the runner folds in the keys of enabled linters, the cache includes the runner's key in every entry's checksum, and the CLI wires them together. The RuboCop linter's key changes when an inherited RuboCop config changes, even though the erb_lint config doesn't. Document `cache_key` for custom linters, since it's their only way to invalidate cached results. Co-Authored-By: Pi <pi@shopify.com>
The RuboCop linter hashed `Gemfile.lock`, so any dependency bump invalidated every cached result even when no RuboCop gem changed. It read the lockfile from the working directory too, which misses it when `BUNDLE_GEMFILE` points elsewhere or erb_lint runs outside Bundler. It also hashed the merged RuboCop config, where RuboCop has expanded every `Exclude` pattern to an absolute path. That made the key differ for every directory the project is checked out in, so checkouts sharing a cache directory, like git worktrees, never hit each other's entries. Hash the versions of the loaded `rubocop*` gems instead of the lockfile, and make `Exclude` patterns relative to the project root. Co-Authored-By: Pi <pi@shopify.com>
`ErbSafety` reads settings like `javascript_safe_methods` from the file named by `better_html_config`, but only the filename is part of the erb_lint config. Marking a helper as safe in that file kept serving the old offenses until the template changed or the cache was cleared. Use the file's contents as the linter's cache key. Co-Authored-By: Pi <pi@shopify.com>
Matching loaded gems on a `rubocop` prefix misses cops that come from anywhere else. Gems pulled in through the config's `require:`, like shopify-cops or smart_todo, and project-local cops under `lib/rubocop/` could change behaviour without invalidating any cached results. RuboCop's result cache already solves this. `ResultCache.source_checksum` hashes the contents of every loaded file plus the RuboCop and rubocop-ast versions, so it covers all of those and stays the same across checkouts. It's memoized on first use, which happens after every linter has loaded its config, and it takes about 10ms over roughly 1,700 files. `source_checksum` only computes anything since RuboCop 1.82.1. Before that it stays nil unless RuboCop's own cache fills it in, so fall back to the `rubocop*` gem versions there. Co-Authored-By: Pi <pi@shopify.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #299. This picks up #373 from @oehlschl (their commit is kept as-is) and addresses the review there, so it should also let us close #300. Loosely related to #463, but they don't depend on each other.
Right now the cache key is the file's mode and contents, the erb_lint config, and the erb_lint version. Editing
.rubocop.ymlor anything it inherits from, bumping RuboCop or a plugin, or editing.better-html.ymlkeeps serving stale results until you clear the cache. I like #373's approach, where each linter can return acache_keyand the runner folds those into the cache key. On top of it, this does 4 things:checksumtocache_keyandset_runner_checksumtorunner_cache_key=, like @etiennebarrie suggested, and adds the specs he asked for, covering each step from a linter's key to a cache miss.RuboCop::ResultCache.source_checksuminstead ofGemfile.lock. Hashing the lockfile has the downside @oehlschl pointed out on Break cache ifGemfile.lockor.rubocop.ymlchange. #300: any dependency bump busts every entry. It's also read from the working directory, so it gets skipped entirely withBUNDLE_GEMFILEor outside Bundler.source_checksumis how RuboCop invalidates its own result cache: it hashes the contents of every loaded file plus the RuboCop and rubocop-ast versions. That covers plugins, cop gems pulled in throughrequire:that aren't namedrubocop-*(like shopify-cops), and project-local cops. It only looks at contents, so it's the same across checkouts, and it takes 10–20ms for roughly 2,000 files.inherit_fromandinherit_gem. RuboCop expands everyExcludein it to an absolute path, though, so the key was different in every checkout. I relativize those against the project root so checkouts sharing a cache directory, like git worktrees, still get hits.ErbSafetykey on the contents of itsbetter_html_configfile, since editing it (e.g. to mark a helper as safe) had the same staleness problem. Let me know if you'd rather I split this out and keep this PR about RuboCop.The tradeoff is that any change to a loaded gem's code now invalidates the cache, which is what RuboCop's own cache does too.
source_checksumis a public method, butResultCacheis marked@api private, and it only computes anything since RuboCop 1.82.1 (rubocop/rubocop#14730). On older versions it staysnil, so the key falls back to the versions of the loadedrubocop*gems. This branch's lockfile is still on RuboCop 1.70, so the spec that edits a file loaded throughrequire:is skipped here until #465 lands.On the note in #373 about busting every cache on release:
ERBLint::VERSIONis already part of the key, so every release does that anyway. I left the runner's key as-is.