Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Implement file cache #268

Merged
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
fbf26a2
Implement file cache
ChrisBr Nov 4, 2020
17ce887
Initial cleaning implementation
zachfeldman Aug 12, 2022
b5aeb01
Rename clean to prune, implement clearing, add autocorrect error message
zachfeldman Aug 13, 2022
95b5e22
Lints and cops
zachfeldman Aug 13, 2022
7032a73
Add integration specs
zachfeldman Aug 13, 2022
6012ae8
Fix cache implementation, clean up code
zachfeldman Aug 14, 2022
ad47b98
Specs for cache and associated changes
zachfeldman Aug 15, 2022
fbbcd2a
Clean up
zachfeldman Aug 15, 2022
a292ef0
Include ERBLint version in cache key
zachfeldman Aug 19, 2022
be11370
Fix nonsensical error message
zachfeldman Aug 19, 2022
deeb4f8
Spacing fixes and removing unneeded code
zachfeldman Aug 19, 2022
1a5db05
Fix tests by mocking returned checksum
zachfeldman Aug 20, 2022
8d4eaa8
Move cache pruning to the cache class
zachfeldman Oct 8, 2022
cdf089c
Do less file reads and stats
zachfeldman Oct 8, 2022
b331799
Attempt a new implementation for storing offenses in a CachedOffense …
zachfeldman Oct 10, 2022
ce8d6da
Update lib/erb_lint/cached_offense.rb to avoid errors on nil severity
zachfeldman Oct 12, 2022
acba4a9
Support caching with json and compact reporters
eterry1388 Oct 12, 2022
6d492e4
Merge pull request #1 from eterry1388/eterry1388/zfeldman/cbruckmayer…
zachfeldman Oct 13, 2022
9ec34e3
Update lib/erb_lint/cli.rb to use safe navigation operator
zachfeldman Oct 13, 2022
877c28c
Rename with-cache to cache
zachfeldman Oct 13, 2022
7ea048c
Prune cache by default
zachfeldman Oct 13, 2022
a32816d
Add README for cache
zachfeldman Oct 13, 2022
e082855
Reduce output of pruning process
zachfeldman Oct 18, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/erb_lint/all.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
require "rubocop"

require "erb_lint"
require "erb_lint/cache"
require "erb_lint/cached_offense"
require "erb_lint/corrector"
require "erb_lint/file_loader"
require "erb_lint/linter_config"
Expand Down
102 changes: 102 additions & 0 deletions lib/erb_lint/cache.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
# frozen_string_literal: true

module ERBLint
class Cache
CACHE_DIRECTORY = ".erb-lint-cache"

def initialize(config, file_loader = nil, prune = false)
@config = config
@file_loader = file_loader
@hits = []
@new_results = []
@prune = prune
puts "Cache mode is on"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

def get(filename, file_content)
file_checksum = checksum(filename, file_content)
begin
cache_file_contents_as_offenses = JSON.parse(
File.read(File.join(CACHE_DIRECTORY, file_checksum))
).map do |offense|
ERBLint::CachedOffense.from_json(offense)
end
rescue Errno::ENOENT
return false
end
@hits.push(file_checksum) if prune?
cache_file_contents_as_offenses
end

def set(filename, file_content, offenses_as_json)
file_checksum = checksum(filename, file_content)
@new_results.push(file_checksum) if prune?

FileUtils.mkdir_p(CACHE_DIRECTORY)

File.open(File.join(CACHE_DIRECTORY, file_checksum), "wb") do |f|
f.write(offenses_as_json)
end
end

def close
prune_cache if prune?
end

def prune_cache
puts "Prune cache mode is on - pruned file names will be logged"
if hits.empty?
puts "Cache being created for the first time, skipping prune"
return
end

cache_files = Dir.new(CACHE_DIRECTORY).children
cache_files.each do |cache_file|
next if hits.include?(cache_file)

if new_results.include?(cache_file)
puts "Skipping deletion of new cache result #{cache_file}"
next
end

puts "Cleaning deleted cached file with checksum #{cache_file}"
File.delete(File.join(CACHE_DIRECTORY, cache_file))
end

@hits = []
end

def cache_dir_exists?
File.directory?(CACHE_DIRECTORY)
end

def clear
return unless cache_dir_exists?

puts "Clearing cache by deleting cache directory"
FileUtils.rm_r(CACHE_DIRECTORY)
end

private

attr_reader :config, :hits, :new_results

def checksum(filename, file_content)
digester = Digest::SHA1.new
mode = File.stat(filename).mode

digester.update(
"#{mode}#{config.to_hash}#{ERBLint::VERSION}#{file_content}"
)
digester.hexdigest
rescue Errno::ENOENT
# Spurious files that come and go should not cause a crash, at least not
# here.
"_"
end

def prune?
@prune
end
end
end
39 changes: 39 additions & 0 deletions lib/erb_lint/cached_offense.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# frozen_string_literal: true

module ERBLint
# A Cached version of an Offense with only essential information represented as strings
class CachedOffense
attr_reader :line_number, :message, :severity

def initialize(message, line_number, severity)
@message = message
@line_number = line_number
@severity = severity
end

def self.new_from_offense(offense)
new(
offense.message,
offense.line_number.to_s,
offense.severity
)
end

def to_json_format
{
message: message,
line_number: line_number,
severity: severity,
}
end

def self.from_json(parsed_json)
parsed_json.transform_keys!(&:to_sym)
new(
parsed_json[:message],
parsed_json[:line_number],
parsed_json[:severity].to_sym
zachfeldman marked this conversation as resolved.
Show resolved Hide resolved
)
end
end
end
77 changes: 72 additions & 5 deletions lib/erb_lint/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,26 @@ def initialize
def run(args = ARGV)
dupped_args = args.dup
load_options(dupped_args)

if with_cache? && autocorrect?
failure!("cannot run autocorrect mode with cache")
end

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

load_config

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

if clear_cache?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if clear_cache?
if clear_cache?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made my own commit to fix these newline suggestions!

if cache.cache_dir_exists?
cache.clear
success!("cache directory cleared")
else
failure!("cache directory doesn't exist, skipping deletion.")
end
end

if !@files.empty? && lint_files.empty?
if allow_no_files?
success!("no files found...\n")
Expand Down Expand Up @@ -65,7 +81,7 @@ def run(args = ARGV)
lint_files.each do |filename|
runner.clear_offenses
begin
file_content = run_with_corrections(runner, filename)
file_content = run_on_file(runner, filename)
rescue => e
@stats.exceptions += 1
puts "Exception occurred when processing: #{relative_filename(filename)}"
Expand All @@ -77,6 +93,8 @@ def run(args = ARGV)
end
end

cache.close if with_cache? || clear_cache?
zachfeldman marked this conversation as resolved.
Show resolved Hide resolved

reporter.show

if stdin? && autocorrect?
Expand All @@ -99,13 +117,47 @@ def run(args = ARGV)

private

attr_reader :cache, :config

def run_on_file(runner, filename)
file_content = read_content(filename)

if with_cache? && !autocorrect?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if with_cache? && !autocorrect?
if with_cache? && !autocorrect?

run_using_cache(runner, filename, file_content)
else
file_content = run_with_corrections(runner, filename, file_content)
end

log_offense_stats(runner, filename)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log_offense_stats(runner, filename)
log_offense_stats(runner, filename)

file_content
end

def run_using_cache(runner, filename, file_content)
if (cache_result_offenses = cache.get(filename, file_content))
runner.restore_offenses(cache_result_offenses)
else
run_with_corrections(runner, filename, file_content)
cache.set(filename, file_content, runner.offenses.map(&:to_cached_offense_json_format).to_json)
end
end

def autocorrect?
@options[:autocorrect]
end

def run_with_corrections(runner, filename)
file_content = read_content(filename)
def with_cache?
@options[:with_cache]
end

def prune_cache?
@options[:prune_cache]
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


def clear_cache?
@options[:clear_cache]
end

def run_with_corrections(runner, filename, file_content)
7.times do
processed_source = ERBLint::ProcessedSource.new(filename, file_content)
runner.run(processed_source)
Expand All @@ -127,6 +179,11 @@ def run_with_corrections(runner, filename)
file_content = corrector.corrected_content
runner.clear_offenses
end

file_content
end

def log_offense_stats(runner, filename)
offenses_filename = relative_filename(filename)
offenses = runner.offenses || []

Expand All @@ -138,8 +195,6 @@ def run_with_corrections(runner, filename)

@stats.processed_files[offenses_filename] ||= []
@stats.processed_files[offenses_filename] |= offenses

file_content
end

def read_content(filename)
Expand Down Expand Up @@ -283,6 +338,18 @@ def option_parser
@options[:enabled_linters] = known_linter_names
end

opts.on("--with-cache", "Enable caching") do |config|
@options[:with_cache] = config
end

opts.on("--prune-cache", "Prune cache") do |config|
@options[:prune_cache] = config
end

opts.on("--clear-cache", "Clear cache") do |config|
@options[:clear_cache] = config
end

opts.on("--enable-linters LINTER[,LINTER,...]", Array,
"Only use specified linter", "Known linters are: #{known_linter_names.join(", ")}") do |linters|
linters.each do |linter|
Expand Down
2 changes: 1 addition & 1 deletion lib/erb_lint/linter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def support_autocorrect?
end
end

attr_reader :offenses
attr_reader :offenses, :config

# Must be implemented by the concrete inheriting class.
def initialize(file_loader, config)
Expand Down
4 changes: 4 additions & 0 deletions lib/erb_lint/offense.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ def initialize(linter, source_range, message, context = nil, severity = nil)
@severity = severity
end

def to_cached_offense_json_format
ERBLint::CachedOffense.new_from_offense(self).to_json_format
end

def inspect
"#<#{self.class.name} linter=#{linter.class.name} "\
"source_range=#{source_range.begin_pos}...#{source_range.end_pos} "\
Expand Down
4 changes: 4 additions & 0 deletions lib/erb_lint/runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,9 @@ def clear_offenses
@offenses = []
@linters.each(&:clear_offenses)
end

def restore_offenses(offenses)
@offenses.concat(offenses)
end
end
end
Loading