From fbf26a29c5f072bd10ac6ada6369ce254c5c206f Mon Sep 17 00:00:00 2001 From: Christian Bruckmayer Date: Wed, 4 Nov 2020 18:08:57 +0000 Subject: [PATCH 01/85] Implement file cache First approach to implement a file cache. https://github.com/Shopify/erb-lint/issues/158 Co-authored-by: Mike Dalessio --- lib/erb_lint.rb | 2 +- lib/erb_lint/all.rb | 1 + lib/erb_lint/cache.rb | 47 +++++++++++++++++++++++++++++++++++++++++++ lib/erb_lint/cli.rb | 33 +++++++++++++++++++++++++++++- 4 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 lib/erb_lint/cache.rb diff --git a/lib/erb_lint.rb b/lib/erb_lint.rb index 66825167..e6afe535 100644 --- a/lib/erb_lint.rb +++ b/lib/erb_lint.rb @@ -1,3 +1,3 @@ # frozen_string_literal: true -require "erb_lint/version" +require "erb_lint/version" \ No newline at end of file diff --git a/lib/erb_lint/all.rb b/lib/erb_lint/all.rb index ecd95cb0..8280343a 100644 --- a/lib/erb_lint/all.rb +++ b/lib/erb_lint/all.rb @@ -3,6 +3,7 @@ require "rubocop" require "erb_lint" +require "erb_lint/cache" require "erb_lint/corrector" require "erb_lint/file_loader" require "erb_lint/linter_config" diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb new file mode 100644 index 00000000..72d16f6f --- /dev/null +++ b/lib/erb_lint/cache.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module ERBLint + class Cache + CACHE_DIRECTORY = '.erb-lint-cache' + private_constant :CACHE_DIRECTORY + + def initialize(config) + @config = config.to_hash + end + + def [](filename) + JSON.parse(File.read(File.join(CACHE_DIRECTORY, checksum(filename)))) + end + + def include?(filename) + File.exist?(File.join(CACHE_DIRECTORY, checksum(filename))) + end + + def []=(filename, messages) + FileUtils.mkdir_p(CACHE_DIRECTORY) + + File.open(File.join(CACHE_DIRECTORY, checksum(filename)), 'wb') do |f| + f.write messages.to_json + end + end + + private + + attr_reader :config + + def checksum(file) + digester = Digest::SHA1.new + mode = File.stat(file).mode + + digester.update( + "#{file}#{mode}#{config.to_s}" + ) + digester.file(file) + digester.hexdigest + rescue Errno::ENOENT + # Spurious files that come and go should not cause a crash, at least not + # here. + '_' + end + end +end diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 7e5396eb..449de305 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -33,6 +33,7 @@ def run(args = ARGV) @files = @options[:stdin] || dupped_args load_config + @cache = Cache.new(config) if !@files.empty? && lint_files.empty? if allow_no_files? @@ -65,7 +66,7 @@ def run(args = ARGV) lint_files.each do |filename| runner.clear_offenses begin - file_content = run_with_corrections(runner, filename) + puts file_content = run_on_file(runner, filename) rescue => e @stats.exceptions += 1 puts "Exception occurred when processing: #{relative_filename(filename)}" @@ -99,10 +100,36 @@ def run(args = ARGV) private + attr_reader :cache, :config + + def run_on_file(runner, filename) + if with_cache? && !autocorrect? + run_using_cache(runner, filename) + else + run_with_corrections(runner, filename) + end + end + + def run_using_cache(runner, filename) + puts cache.include?(filename) + if cache.include?(filename) && !autocorrect? + result = cache[filename] + @stats.found += result.size + result + else + result = run_with_corrections(runner, filename) + cache[filename] = result + end + end + def autocorrect? @options[:autocorrect] end + def with_cache? + @options[:with_cache] + end + def run_with_corrections(runner, filename) file_content = read_content(filename) @@ -283,6 +310,10 @@ 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("--enable-linters LINTER[,LINTER,...]", Array, "Only use specified linter", "Known linters are: #{known_linter_names.join(", ")}") do |linters| linters.each do |linter| From 17ce88729da60df5fec50e1679ede38d0d9a6cf3 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Fri, 12 Aug 2022 16:15:20 -0400 Subject: [PATCH 02/85] Initial cleaning implementation --- Gemfile | 1 + lib/erb_lint/cache.rb | 35 ++++++++++++++++++++++++++++++++++- lib/erb_lint/cli.rb | 18 +++++++++++++++--- 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index 548e391e..efd1b411 100644 --- a/Gemfile +++ b/Gemfile @@ -3,6 +3,7 @@ source "https://rubygems.org" gem "rake" +gem 'pry' group "test" do gem "fakefs" diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 72d16f6f..3416c8cd 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -7,6 +7,8 @@ class Cache def initialize(config) @config = config.to_hash + @hits = [] + @new_results = [] end def [](filename) @@ -25,9 +27,40 @@ def []=(filename, messages) end end + def add_hit(hit) + @hits.push(hit) + end + + def add_new_result(filename) + @new_results.push(filename) + end + + def clean + if hits.empty? + puts "Cache being created, skipping clean" + return + end + + cache_files = Dir.new(CACHE_DIRECTORY).children + hits_as_checksums = hits.map{|hit| checksum(hit) } + new_results_as_checksums = new_results.map{|new_result| checksum(new_result) } + cache_files.each do |cache_file| + next if hits_as_checksums.include?(cache_file) + if new_results_as_checksums.include?(cache_file) + puts "Skipping deletion of new cache result #{cache_file} in clean" + return + end + + puts "Cleaning deleted cached file with checksum #{cache_file}}" + File.delete(File.join(CACHE_DIRECTORY, cache_file)) + end + + @hits = [] + end + private - attr_reader :config + attr_reader :config, :hits, :new_results def checksum(file) digester = Digest::SHA1.new diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 449de305..7270d745 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -66,7 +66,7 @@ def run(args = ARGV) lint_files.each do |filename| runner.clear_offenses begin - puts file_content = run_on_file(runner, filename) + file_content = run_on_file(runner, filename) rescue => e @stats.exceptions += 1 puts "Exception occurred when processing: #{relative_filename(filename)}" @@ -78,6 +78,8 @@ def run(args = ARGV) end end + cache.clean if clean_cache? + reporter.show if stdin? && autocorrect? @@ -111,14 +113,16 @@ def run_on_file(runner, filename) end def run_using_cache(runner, filename) - puts cache.include?(filename) + # puts cache.include?(filename) if cache.include?(filename) && !autocorrect? result = cache[filename] @stats.found += result.size + cache.add_hit(filename) if clean_cache? result else result = run_with_corrections(runner, filename) cache[filename] = result + cache.add_new_result(filename) if clean_cache? end end @@ -130,6 +134,10 @@ def with_cache? @options[:with_cache] end + def clean_cache? + @options[:clean_cache] + end + def run_with_corrections(runner, filename) file_content = read_content(filename) @@ -310,10 +318,14 @@ def option_parser @options[:enabled_linters] = known_linter_names end - opts.on("--with_cache", "Enable caching") do |config| + opts.on("--with-cache", "Enable caching") do |config| @options[:with_cache] = config end + opts.on("--clean-cache", "Clean cache") do |config| + @options[:clean_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| From b5aeb01ec9f42e55ce061a9ebd8c57050144ef99 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Sat, 13 Aug 2022 14:08:30 -0400 Subject: [PATCH 03/85] Rename clean to prune, implement clearing, add autocorrect error message --- lib/erb_lint/cache.rb | 16 +++++++++++++--- lib/erb_lint/cli.rb | 36 +++++++++++++++++++++++++++--------- 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 3416c8cd..1497948d 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -35,9 +35,9 @@ def add_new_result(filename) @new_results.push(filename) end - def clean + def prune if hits.empty? - puts "Cache being created, skipping clean" + puts "Cache being created for the first time, skipping prune" return end @@ -47,7 +47,7 @@ def clean cache_files.each do |cache_file| next if hits_as_checksums.include?(cache_file) if new_results_as_checksums.include?(cache_file) - puts "Skipping deletion of new cache result #{cache_file} in clean" + puts "Skipping deletion of new cache result #{cache_file} in prune" return end @@ -58,6 +58,16 @@ def clean @hits = [] end + def clear + puts "Clearing cache by deleting cache directory..." + begin + FileUtils.remove_dir(CACHE_DIRECTORY) + puts "...cache directory cleared" + rescue Errno::ENOENT => e + puts "...directory already doesn't exist, skipped deletion." + end + end + private attr_reader :config, :hits, :new_results diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 7270d745..3e57f046 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -33,7 +33,18 @@ def run(args = ARGV) @files = @options[:stdin] || dupped_args load_config - @cache = Cache.new(config) + + if with_cache? && autocorrect? + failure!("cannot run autocorrect mode with cache") + end + + @cache = Cache.new(config) if with_cache? || clear_cache? + if clear_cache? + cache.clear + exit 0 + end + + if !@files.empty? && lint_files.empty? if allow_no_files? @@ -78,7 +89,7 @@ def run(args = ARGV) end end - cache.clean if clean_cache? + cache.prune if prune_cache? reporter.show @@ -116,13 +127,12 @@ def run_using_cache(runner, filename) # puts cache.include?(filename) if cache.include?(filename) && !autocorrect? result = cache[filename] - @stats.found += result.size - cache.add_hit(filename) if clean_cache? + cache.add_hit(filename) if prune_cache? result else result = run_with_corrections(runner, filename) cache[filename] = result - cache.add_new_result(filename) if clean_cache? + cache.add_new_result(filename) if prune_cache? end end @@ -134,8 +144,12 @@ def with_cache? @options[:with_cache] end - def clean_cache? - @options[:clean_cache] + def prune_cache? + @options[:prune_cache] + end + + def clear_cache? + @options[:clear_cache] end def run_with_corrections(runner, filename) @@ -322,8 +336,12 @@ def option_parser @options[:with_cache] = config end - opts.on("--clean-cache", "Clean cache") do |config| - @options[:clean_cache] = config + 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, From 95b5e224cf484ad7bcd772281c1c52a78ff3b969 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Sat, 13 Aug 2022 14:10:24 -0400 Subject: [PATCH 04/85] Lints and cops --- Gemfile | 2 +- Gemfile.lock | 6 ++++++ lib/erb_lint.rb | 2 +- lib/erb_lint/cache.rb | 23 ++++++++++++----------- lib/erb_lint/cli.rb | 4 +--- 5 files changed, 21 insertions(+), 16 deletions(-) diff --git a/Gemfile b/Gemfile index efd1b411..3ef7f8e2 100644 --- a/Gemfile +++ b/Gemfile @@ -3,7 +3,7 @@ source "https://rubygems.org" gem "rake" -gem 'pry' +gem "pry" group "test" do gem "fakefs" diff --git a/Gemfile.lock b/Gemfile.lock index 72c45979..b5d2ec89 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -32,6 +32,7 @@ GEM parser (>= 2.4) smart_properties builder (3.2.4) + coderay (1.1.3) concurrent-ruby (1.1.10) crass (1.0.6) diff-lcs (1.5.0) @@ -42,6 +43,7 @@ GEM loofah (2.18.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) + method_source (1.0.0) mini_portile2 (2.8.0) minitest (5.16.3) nokogiri (1.13.8) @@ -50,6 +52,9 @@ GEM parallel (1.22.1) parser (3.1.2.1) ast (~> 2.4.1) + pry (0.14.1) + coderay (~> 1.1) + method_source (~> 1.0) racc (1.6.0) rails-dom-testing (2.0.3) activesupport (>= 4.2.0) @@ -98,6 +103,7 @@ PLATFORMS DEPENDENCIES erb_lint! fakefs + pry rake rspec rubocop diff --git a/lib/erb_lint.rb b/lib/erb_lint.rb index e6afe535..66825167 100644 --- a/lib/erb_lint.rb +++ b/lib/erb_lint.rb @@ -1,3 +1,3 @@ # frozen_string_literal: true -require "erb_lint/version" \ No newline at end of file +require "erb_lint/version" diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 1497948d..4bf13f06 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -2,7 +2,7 @@ module ERBLint class Cache - CACHE_DIRECTORY = '.erb-lint-cache' + CACHE_DIRECTORY = ".erb-lint-cache" private_constant :CACHE_DIRECTORY def initialize(config) @@ -14,16 +14,16 @@ def initialize(config) def [](filename) JSON.parse(File.read(File.join(CACHE_DIRECTORY, checksum(filename)))) end - + def include?(filename) File.exist?(File.join(CACHE_DIRECTORY, checksum(filename))) end - + def []=(filename, messages) FileUtils.mkdir_p(CACHE_DIRECTORY) - File.open(File.join(CACHE_DIRECTORY, checksum(filename)), 'wb') do |f| - f.write messages.to_json + File.open(File.join(CACHE_DIRECTORY, checksum(filename)), "wb") do |f| + f.write(messages.to_json) end end @@ -42,13 +42,14 @@ def prune end cache_files = Dir.new(CACHE_DIRECTORY).children - hits_as_checksums = hits.map{|hit| checksum(hit) } - new_results_as_checksums = new_results.map{|new_result| checksum(new_result) } + hits_as_checksums = hits.map { |hit| checksum(hit) } + new_results_as_checksums = new_results.map { |new_result| checksum(new_result) } cache_files.each do |cache_file| next if hits_as_checksums.include?(cache_file) + if new_results_as_checksums.include?(cache_file) puts "Skipping deletion of new cache result #{cache_file} in prune" - return + next end puts "Cleaning deleted cached file with checksum #{cache_file}}" @@ -63,7 +64,7 @@ def clear begin FileUtils.remove_dir(CACHE_DIRECTORY) puts "...cache directory cleared" - rescue Errno::ENOENT => e + rescue Errno::ENOENT puts "...directory already doesn't exist, skipped deletion." end end @@ -77,14 +78,14 @@ def checksum(file) mode = File.stat(file).mode digester.update( - "#{file}#{mode}#{config.to_s}" + "#{file}#{mode}#{config}" ) digester.file(file) digester.hexdigest rescue Errno::ENOENT # Spurious files that come and go should not cause a crash, at least not # here. - '_' + "_" end end end diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 3e57f046..9f18aabb 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -41,11 +41,9 @@ def run(args = ARGV) @cache = Cache.new(config) if with_cache? || clear_cache? if clear_cache? cache.clear - exit 0 + exit(0) end - - if !@files.empty? && lint_files.empty? if allow_no_files? success!("no files found...\n") From 7032a73ee0ed805473a0d0b5c7456d618774b2df Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Sat, 13 Aug 2022 15:59:37 -0400 Subject: [PATCH 05/85] Add integration specs --- lib/erb_lint/cache.rb | 17 +++++----- lib/erb_lint/cli.rb | 15 ++++++--- spec/erb_lint/cli_spec.rb | 68 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 86 insertions(+), 14 deletions(-) diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 4bf13f06..7e21c63e 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -3,12 +3,12 @@ module ERBLint class Cache CACHE_DIRECTORY = ".erb-lint-cache" - private_constant :CACHE_DIRECTORY def initialize(config) @config = config.to_hash @hits = [] @new_results = [] + puts "Cache mode is on" end def [](filename) @@ -59,14 +59,15 @@ def prune @hits = [] end + def cache_dir_exists? + File.directory?(CACHE_DIRECTORY) + end + def clear - puts "Clearing cache by deleting cache directory..." - begin - FileUtils.remove_dir(CACHE_DIRECTORY) - puts "...cache directory cleared" - rescue Errno::ENOENT - puts "...directory already doesn't exist, skipped deletion." - end + return unless cache_dir_exists? + + puts "Clearing cache by deleting cache directory" + FileUtils.rm_r(CACHE_DIRECTORY) end private diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 9f18aabb..c4f6c7ab 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -30,18 +30,23 @@ def initialize def run(args = ARGV) dupped_args = args.dup load_options(dupped_args) - @files = @options[:stdin] || dupped_args - - load_config if with_cache? && autocorrect? failure!("cannot run autocorrect mode with cache") end + @files = @options[:stdin] || dupped_args + + load_config + @cache = Cache.new(config) if with_cache? || clear_cache? if clear_cache? - cache.clear - exit(0) + if cache.cache_dir_exists? + cache.clear + success!("cache directory cleared") + else + failure!("cache directory already doesn't exist, skipped deletion.") + end end if !@files.empty? && lint_files.empty? diff --git a/spec/erb_lint/cli_spec.rb b/spec/erb_lint/cli_spec.rb index 77c61b5b..d282b131 100644 --- a/spec/erb_lint/cli_spec.rb +++ b/spec/erb_lint/cli_spec.rb @@ -2,6 +2,7 @@ require "spec_helper" require "erb_lint/cli" +require "erb_lint/cache" require "pp" require "fakefs" require "fakefs/spec_helpers" @@ -97,6 +98,34 @@ def run(processed_source) end end + context "with --clear-cache" do + let(:args) { ["--clear-cache"] } + context "without a cache folder" do + it { expect { subject }.to(output(/cache directory already doesn't exist, skipped deletion/).to_stderr) } + it "shows cache not cleared message if cache is empty and fails" do + expect(subject).to(be(false)) + end + end + + context "with a cache folder" do + before do + FileUtils.mkdir_p(ERBLint::Cache::CACHE_DIRECTORY) + end + it { + expect do + subject + end.to(output(<<~EOF).to_stdout) + Cache mode is on + Clearing cache by deleting cache directory + cache directory cleared + EOF + } + it "is successful and empties the cache if there are cache file" do + expect(subject).to(be(true)) + end + end + end + context "with --config" do context "when file does not exist" do let(:args) { ["--config", ".somefile.yml"] } @@ -170,7 +199,7 @@ def run(processed_source) end end - context "when only errors with serverity info are found" do + context "when only errors with severity info are found" do let(:args) { ["--enable-linter", "linter_with_info_errors", linted_file] } it "shows all error messages and line numbers" do @@ -218,6 +247,19 @@ def run(processed_source) expect(subject).to(be(false)) end end + + context "with cache" do + let(:args) { ["--enable-linter", "linter_without_errors", "--with-cache", linted_file] } + + it "lints the file and adds it to the cache" do + expect(Dir[ERBLint::Cache::CACHE_DIRECTORY].length).to(be(0)) + + expect { subject }.to(output(/Cache mode is on/).to_stdout) + expect(subject).to(be(true)) + + expect(Dir[ERBLint::Cache::CACHE_DIRECTORY].length).to(be(1)) + end + end end end @@ -428,6 +470,19 @@ def run(processed_source) it "is successful" do expect(subject).to(be(true)) end + + context "with cache" do + let(:args) { ["--enable-linter", "linter_without_errors", "--with-cache", linted_dir] } + + it "lints the file and adds it to the cache" do + expect(Dir[ERBLint::Cache::CACHE_DIRECTORY].length).to(be(0)) + + expect { subject }.to(output(/Cache mode is on/).to_stdout) + expect(subject).to(be(true)) + + expect(Dir[ERBLint::Cache::CACHE_DIRECTORY].length).to(be(1)) + end + end end end end @@ -531,6 +586,17 @@ def run(processed_source) expect { subject }.to(output(/#{file_content}\n/).to_stdout) end end + + context "when autocorrecting and caching are turned on" do + # We assume that linter_with_errors is not autocorrectable... + let(:args) do + ["--enable-linter", "linter_without_errors", "--stdin", linted_file, "--autocorrect", "--with-cache"] + end + + it "throws an error saying the two modes cannot be used together" do + expect { subject }.to(output(/cannot run autocorrect mode with cache/).to_stderr) + end + end end context "when no errors are found" do From 6012ae87913ed0e0517ec1acf07766bbe35977fa Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Sun, 14 Aug 2022 13:41:52 -0400 Subject: [PATCH 06/85] Fix cache implementation, clean up code --- lib/erb_lint/cache.rb | 17 +++++++++------- lib/erb_lint/cli.rb | 29 ++++++++++++++------------- lib/erb_lint/linter.rb | 2 +- lib/erb_lint/offense.rb | 43 +++++++++++++++++++++++++++++++++++++++++ lib/erb_lint/runner.rb | 4 ++++ 5 files changed, 74 insertions(+), 21 deletions(-) diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 7e21c63e..6fb0e0c0 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -4,26 +4,29 @@ module ERBLint class Cache CACHE_DIRECTORY = ".erb-lint-cache" - def initialize(config) - @config = config.to_hash + def initialize(config, file_loader = nil) + @config = config + @file_loader = file_loader @hits = [] @new_results = [] puts "Cache mode is on" end def [](filename) - JSON.parse(File.read(File.join(CACHE_DIRECTORY, checksum(filename)))) + JSON.parse(File.read(File.join(CACHE_DIRECTORY, checksum(filename)))).map do |offense| + ERBLint::Offense.from_json(offense, @file_loader, config) + end end def include?(filename) File.exist?(File.join(CACHE_DIRECTORY, checksum(filename))) end - def []=(filename, messages) + def []=(filename, offenses_as_json) FileUtils.mkdir_p(CACHE_DIRECTORY) File.open(File.join(CACHE_DIRECTORY, checksum(filename)), "wb") do |f| - f.write(messages.to_json) + f.write(offenses_as_json) end end @@ -52,7 +55,7 @@ def prune next end - puts "Cleaning deleted cached file with checksum #{cache_file}}" + puts "Cleaning deleted cached file with checksum #{cache_file}" File.delete(File.join(CACHE_DIRECTORY, cache_file)) end @@ -79,7 +82,7 @@ def checksum(file) mode = File.stat(file).mode digester.update( - "#{file}#{mode}#{config}" + "#{file}#{mode}#{config.to_hash}" ) digester.file(file) digester.hexdigest diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index c4f6c7ab..15aa2673 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -39,7 +39,7 @@ def run(args = ARGV) load_config - @cache = Cache.new(config) if with_cache? || clear_cache? + @cache = Cache.new(@config, file_loader) if with_cache? || clear_cache? if clear_cache? if cache.cache_dir_exists? cache.clear @@ -119,22 +119,24 @@ def run(args = ARGV) attr_reader :cache, :config def run_on_file(runner, filename) + file_content = read_content(filename) if with_cache? && !autocorrect? - run_using_cache(runner, filename) + run_using_cache(runner, filename, file_content) else - run_with_corrections(runner, filename) + file_content = run_with_corrections(runner, filename, file_content) end + log_offense_stats(runner, filename) + file_content end - def run_using_cache(runner, filename) + def run_using_cache(runner, filename, file_content) # puts cache.include?(filename) if cache.include?(filename) && !autocorrect? - result = cache[filename] + runner.restore_offenses(cache[filename]) cache.add_hit(filename) if prune_cache? - result else - result = run_with_corrections(runner, filename) - cache[filename] = result + run_with_corrections(runner, filename, file_content) + cache[filename] = runner.offenses.map(&:to_json_format).to_json cache.add_new_result(filename) if prune_cache? end end @@ -155,9 +157,7 @@ def clear_cache? @options[:clear_cache] end - def run_with_corrections(runner, filename) - file_content = read_content(filename) - + def run_with_corrections(runner, filename, file_content) 7.times do processed_source = ERBLint::ProcessedSource.new(filename, file_content) runner.run(processed_source) @@ -179,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 || [] @@ -190,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) diff --git a/lib/erb_lint/linter.rb b/lib/erb_lint/linter.rb index 36c62511..3d452c90 100644 --- a/lib/erb_lint/linter.rb +++ b/lib/erb_lint/linter.rb @@ -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) diff --git a/lib/erb_lint/offense.rb b/lib/erb_lint/offense.rb index dc66014c..d730f9e4 100644 --- a/lib/erb_lint/offense.rb +++ b/lib/erb_lint/offense.rb @@ -17,6 +17,49 @@ def initialize(linter, source_range, message, context = nil, severity = nil) @severity = severity end + def to_json_format + { + linter_config: linter.config.to_hash, + source_range: source_range_to_json_format, + message: message, + context: context, + severity: severity, + } + end + + def source_range_to_json_format + { + begin_pos: source_range.begin_pos, + end_pos: source_range.end_pos, + source_buffer_source: source_range.source_buffer.source, + source_buffer_name: source_range.source_buffer.name, + } + end + + def self.from_json(parsed_json, file_loader, config) + parsed_json.transform_keys!(&:to_sym) + linter_config = ERBLint::LinterConfig.new(parsed_json[:linter_config]) + new( + linter_config, + source_range_from_json_format(parsed_json[:source_range]), + parsed_json[:message].presence, + parsed_json[:context].presence, + parsed_json[:severity].presence + ) + end + + def self.source_range_from_json_format(parsed_json_source_range) + parsed_json_source_range.transform_keys!(&:to_sym) + Parser::Source::Range.new( + Parser::Source::Buffer.new( + parsed_json_source_range[:source_buffer_name], + source: parsed_json_source_range[:source_buffer_source] + ), + parsed_json_source_range[:begin_pos], + parsed_json_source_range[:end_pos] + ) + end + def inspect "#<#{self.class.name} linter=#{linter.class.name} "\ "source_range=#{source_range.begin_pos}...#{source_range.end_pos} "\ diff --git a/lib/erb_lint/runner.rb b/lib/erb_lint/runner.rb index 3965872c..f769f25a 100644 --- a/lib/erb_lint/runner.rb +++ b/lib/erb_lint/runner.rb @@ -30,5 +30,9 @@ def clear_offenses @offenses = [] @linters.each(&:clear_offenses) end + + def restore_offenses(offenses) + @offenses.concat(offenses) + end end end From ad47b9832df9a71da9f65ffd52100f4e453688b7 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Sun, 14 Aug 2022 20:09:07 -0400 Subject: [PATCH 07/85] Specs for cache and associated changes --- lib/erb_lint/cache.rb | 4 +- lib/erb_lint/cli.rb | 2 +- lib/erb_lint/offense.rb | 18 +- spec/erb_lint/cache_spec.rb | 172 ++++++++++++++++++ spec/erb_lint/fixtures/cache_file_content | 1 + .../fixtures/image_component.html.erb | 11 ++ spec/erb_lint/fixtures/rubocop.yml | 2 + 7 files changed, 199 insertions(+), 11 deletions(-) create mode 100644 spec/erb_lint/cache_spec.rb create mode 100644 spec/erb_lint/fixtures/cache_file_content create mode 100644 spec/erb_lint/fixtures/image_component.html.erb create mode 100644 spec/erb_lint/fixtures/rubocop.yml diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 6fb0e0c0..5547da1a 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -12,9 +12,9 @@ def initialize(config, file_loader = nil) puts "Cache mode is on" end - def [](filename) + def get(filename, file_content) JSON.parse(File.read(File.join(CACHE_DIRECTORY, checksum(filename)))).map do |offense| - ERBLint::Offense.from_json(offense, @file_loader, config) + ERBLint::Offense.from_json(offense, config, @file_loader, file_content) end end diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 15aa2673..7d9a7a7c 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -132,7 +132,7 @@ def run_on_file(runner, filename) def run_using_cache(runner, filename, file_content) # puts cache.include?(filename) if cache.include?(filename) && !autocorrect? - runner.restore_offenses(cache[filename]) + runner.restore_offenses(cache.get(filename, file_content)) cache.add_hit(filename) if prune_cache? else run_with_corrections(runner, filename, file_content) diff --git a/lib/erb_lint/offense.rb b/lib/erb_lint/offense.rb index d730f9e4..8c306239 100644 --- a/lib/erb_lint/offense.rb +++ b/lib/erb_lint/offense.rb @@ -20,6 +20,8 @@ def initialize(linter, source_range, message, context = nil, severity = nil) def to_json_format { linter_config: linter.config.to_hash, + linter_config_class: linter.config.class.name, + linter_class: linter.class.name, source_range: source_range_to_json_format, message: message, context: context, @@ -31,29 +33,29 @@ def source_range_to_json_format { begin_pos: source_range.begin_pos, end_pos: source_range.end_pos, - source_buffer_source: source_range.source_buffer.source, source_buffer_name: source_range.source_buffer.name, } end - def self.from_json(parsed_json, file_loader, config) + def self.from_json(parsed_json, config, file_loader, file_content) parsed_json.transform_keys!(&:to_sym) - linter_config = ERBLint::LinterConfig.new(parsed_json[:linter_config]) + linter_config = const_get(parsed_json[:linter_config_class]).new(parsed_json[:linter_config]) + linter = const_get(parsed_json[:linter_class]).new(file_loader, linter_config) new( - linter_config, - source_range_from_json_format(parsed_json[:source_range]), + linter, + source_range_from_json_format(parsed_json[:source_range], file_content), parsed_json[:message].presence, parsed_json[:context].presence, - parsed_json[:severity].presence + parsed_json[:severity].presence&.to_sym ) end - def self.source_range_from_json_format(parsed_json_source_range) + def self.source_range_from_json_format(parsed_json_source_range, file_content) parsed_json_source_range.transform_keys!(&:to_sym) Parser::Source::Range.new( Parser::Source::Buffer.new( parsed_json_source_range[:source_buffer_name], - source: parsed_json_source_range[:source_buffer_source] + source: file_content ), parsed_json_source_range[:begin_pos], parsed_json_source_range[:end_pos] diff --git a/spec/erb_lint/cache_spec.rb b/spec/erb_lint/cache_spec.rb new file mode 100644 index 00000000..3f6e542f --- /dev/null +++ b/spec/erb_lint/cache_spec.rb @@ -0,0 +1,172 @@ +# frozen_string_literal: true + +require "spec_helper" + +require "fakefs" +require "fakefs/spec_helpers" +require "erb_lint/cache" + +require "pry" + +describe ERBLint::Cache do + include FakeFS::SpecHelpers + + let(:linter_config) { ERBLint::LinterConfig.new } + let(:cache) { described_class.new(linter_config) } + let(:linted_file_path) { "app/components/elements/image_component/image_component.html.erb" } + let(:file_content) do + %() + end + let(:checksum) { "835c15465bc22783257bdafb33acc2d78c29abaa" } + let(:cache_dir) { ERBLint::Cache::CACHE_DIRECTORY } + let(:rubocop_yml) { %(SpaceAroundErbTag:\n Enabled: true\n) } + let(:cache_file_content) do + FakeFS.deactivate! + content = File.read(File.expand_path("./spec/erb_lint/fixtures/cache_file_content")) + FakeFS.activate! + content + end + let(:linted_file_content) do + FakeFS.deactivate! + content = File.read(File.expand_path("./spec/erb_lint/fixtures/image_component.html.erb")) + FakeFS.activate! + content + end + + around do |example| + FakeFS do + example.run + end + end + + before do + FileUtils.mkdir_p(File.dirname(linted_file_path)) + File.write(linted_file_path, linted_file_content) + + FileUtils.mkdir_p(cache_dir) + File.open(File.join(cache_dir, checksum), "wb") do |f| + f.write(cache_file_content) + end + + allow(::RuboCop::ConfigLoader).to(receive(:load_file).and_call_original) + allow(::RuboCop::ConfigLoader).to(receive(:read_file).and_return(rubocop_yml)) + end + + describe "#get" do + it "returns a cached lint result" do + cache_result = cache.get(linted_file_path, cache_file_content) + expect(cache_result.count).to(eq(2)) + end + end + + describe "#[]=" do + it "caches a lint result" do + cache[linted_file_path] = cache_file_content + expect(File.exist?( + File.join( + cache_dir, + checksum + ) + )).to(be(true)) + end + end + + describe "#include?" do + it "returns true if the cache includes the filename" do + expect(cache.include?(linted_file_path)).to(be(true)) + end + + it "returns false if the cache does not include the filename" do + expect(cache.include?("gibberish")).to(be(false)) + end + end + + describe "#cache_dir_exists?" do + it "returns true if the cache dir exists" do + expect(cache.cache_dir_exists?).to(be(true)) + end + it "returns false if the cache dir does not exist" do + FileUtils.rm_rf(cache_dir) + expect(cache.cache_dir_exists?).to(be(false)) + end + end + + describe "#clear" do + it "deletes the cache directory" do + cache.clear + expect(File.directory?(cache_dir)).to(be(false)) + end + end + + describe "#prune" do + it "skips prune if no cache hits" do + allow(cache).to(receive(:hits).and_return([])) + + expect { cache.prune }.to(output(/Cache being created for the first time, skipping prune/).to_stdout) + end + + it "does not prune actual cache hits" do + cache.prune + + expect(File.exist?( + File.join( + cache_dir, + checksum + ) + )).to(be(true)) + end + + it "does not prune new cache results" do + allow(cache).to(receive(:hits).and_return(["fake-hit"])) + allow(cache).to(receive(:new_results).and_return([linted_file_path])) + fakefs_dir = Struct.new(:fakefs_dir) + allow(fakefs_dir).to(receive(:children).and_return([checksum])) + allow(FakeFS::Dir).to(receive(:new).and_return(fakefs_dir)) + + expect { cache.prune }.to(output(/Skipping deletion of new cache result #{checksum} in prune/).to_stdout) + + expect(File.exist?( + File.join( + cache_dir, + checksum + ) + )).to(be(true)) + end + + it "prunes outdated cache results" do + fakefs_dir = Struct.new(:fakefs_dir) + allow(fakefs_dir).to(receive(:children).and_return([checksum, "fake-checksum"])) + allow(FakeFS::Dir).to(receive(:new).and_return(fakefs_dir)) + allow(cache).to(receive(:hits).and_return([linted_file_path])) + + File.open(File.join(cache_dir, "fake-checksum"), "wb") do |f| + f.write(cache_file_content) + end + + expect { cache.prune }.to(output(/Cleaning deleted cached file with checksum fake-checksum/).to_stdout) + + expect(File.exist?( + File.join( + cache_dir, + "fake-checksum" + ) + )).to(be(false)) + end + end + + describe "#add_new_result" do + it "adds new result to cache object new_results attribute" do + cache.add_new_result(linted_file_path) + + expect(cache.send(:new_results)).to(include(linted_file_path)) + end + end + + describe "#add_hit" do + it "adds new cache hit to cache object hits attribute" do + cache.add_hit(linted_file_path) + + expect(cache.send(:hits)).to(include(linted_file_path)) + end + end +end diff --git a/spec/erb_lint/fixtures/cache_file_content b/spec/erb_lint/fixtures/cache_file_content new file mode 100644 index 00000000..e044f385 --- /dev/null +++ b/spec/erb_lint/fixtures/cache_file_content @@ -0,0 +1 @@ +[{"linter_config":{"enabled":true,"exclude":[]},"linter_config_class":"ERBLint::LinterConfig","linter_class":"ERBLint::Linters::SpaceAroundErbTag","source_range":{"begin_pos":2,"end_pos":2,"source_buffer_name":"/home/zach/dev/fake-app/app/components/elements/image_component/image_component.html.erb"},"message":"Use 1 space after `<%` instead of 0 space.","context":" ","severity":null},{"linter_config":{"enabled":true,"exclude":[]},"linter_config_class":"ERBLint::LinterConfig","linter_class":"ERBLint::Linters::SpaceAroundErbTag","source_range":{"begin_pos":172,"end_pos":172,"source_buffer_name":"/home/zach/dev/fake-app/app/components/elements/image_component/image_component.html.erb"},"message":"Use 1 space before `%>` instead of 0 space.","context":" ","severity":null}] \ No newline at end of file diff --git a/spec/erb_lint/fixtures/image_component.html.erb b/spec/erb_lint/fixtures/image_component.html.erb new file mode 100644 index 00000000..a74c44ba --- /dev/null +++ b/spec/erb_lint/fixtures/image_component.html.erb @@ -0,0 +1,11 @@ +<%if variable %> +
+
+ <%= render_image %> +
+ <%= link_to "Hello World", root_path %> +
+
+<% else%> + <%= render_image %> +<% end %> \ No newline at end of file diff --git a/spec/erb_lint/fixtures/rubocop.yml b/spec/erb_lint/fixtures/rubocop.yml new file mode 100644 index 00000000..fc768c91 --- /dev/null +++ b/spec/erb_lint/fixtures/rubocop.yml @@ -0,0 +1,2 @@ +SpaceAroundErbTag: + Enabled: true From fbbcd2a218c831a0d7c33e645dcecb131ceba5df Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Sun, 14 Aug 2022 20:15:37 -0400 Subject: [PATCH 08/85] Clean up --- Gemfile | 1 - Gemfile.lock | 6 ------ lib/erb_lint/cli.rb | 1 - spec/erb_lint/cache_spec.rb | 8 +------- 4 files changed, 1 insertion(+), 15 deletions(-) diff --git a/Gemfile b/Gemfile index 3ef7f8e2..548e391e 100644 --- a/Gemfile +++ b/Gemfile @@ -3,7 +3,6 @@ source "https://rubygems.org" gem "rake" -gem "pry" group "test" do gem "fakefs" diff --git a/Gemfile.lock b/Gemfile.lock index b5d2ec89..72c45979 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -32,7 +32,6 @@ GEM parser (>= 2.4) smart_properties builder (3.2.4) - coderay (1.1.3) concurrent-ruby (1.1.10) crass (1.0.6) diff-lcs (1.5.0) @@ -43,7 +42,6 @@ GEM loofah (2.18.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) - method_source (1.0.0) mini_portile2 (2.8.0) minitest (5.16.3) nokogiri (1.13.8) @@ -52,9 +50,6 @@ GEM parallel (1.22.1) parser (3.1.2.1) ast (~> 2.4.1) - pry (0.14.1) - coderay (~> 1.1) - method_source (~> 1.0) racc (1.6.0) rails-dom-testing (2.0.3) activesupport (>= 4.2.0) @@ -103,7 +98,6 @@ PLATFORMS DEPENDENCIES erb_lint! fakefs - pry rake rspec rubocop diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 7d9a7a7c..d811f9a9 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -130,7 +130,6 @@ def run_on_file(runner, filename) end def run_using_cache(runner, filename, file_content) - # puts cache.include?(filename) if cache.include?(filename) && !autocorrect? runner.restore_offenses(cache.get(filename, file_content)) cache.add_hit(filename) if prune_cache? diff --git a/spec/erb_lint/cache_spec.rb b/spec/erb_lint/cache_spec.rb index 3f6e542f..a4098dc9 100644 --- a/spec/erb_lint/cache_spec.rb +++ b/spec/erb_lint/cache_spec.rb @@ -1,12 +1,9 @@ # frozen_string_literal: true require "spec_helper" - +require "erb_lint/cache" require "fakefs" require "fakefs/spec_helpers" -require "erb_lint/cache" - -require "pry" describe ERBLint::Cache do include FakeFS::SpecHelpers @@ -14,9 +11,6 @@ let(:linter_config) { ERBLint::LinterConfig.new } let(:cache) { described_class.new(linter_config) } let(:linted_file_path) { "app/components/elements/image_component/image_component.html.erb" } - let(:file_content) do - %() - end let(:checksum) { "835c15465bc22783257bdafb33acc2d78c29abaa" } let(:cache_dir) { ERBLint::Cache::CACHE_DIRECTORY } let(:rubocop_yml) { %(SpaceAroundErbTag:\n Enabled: true\n) } From a292ef095c4726ea437d6034bee9f6df2d579e6f Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Thu, 18 Aug 2022 20:13:26 -0400 Subject: [PATCH 09/85] Include ERBLint version in cache key --- lib/erb_lint/cache.rb | 2 +- spec/erb_lint/cache_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 5547da1a..a2760e0d 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -82,7 +82,7 @@ def checksum(file) mode = File.stat(file).mode digester.update( - "#{file}#{mode}#{config.to_hash}" + "#{file}#{mode}#{config.to_hash}#{ERBLint::VERSION}" ) digester.file(file) digester.hexdigest diff --git a/spec/erb_lint/cache_spec.rb b/spec/erb_lint/cache_spec.rb index a4098dc9..ce7dde19 100644 --- a/spec/erb_lint/cache_spec.rb +++ b/spec/erb_lint/cache_spec.rb @@ -11,7 +11,7 @@ let(:linter_config) { ERBLint::LinterConfig.new } let(:cache) { described_class.new(linter_config) } let(:linted_file_path) { "app/components/elements/image_component/image_component.html.erb" } - let(:checksum) { "835c15465bc22783257bdafb33acc2d78c29abaa" } + let(:checksum) { "2dc3e17183b87889cc783b0157723570d4bbb90a" } let(:cache_dir) { ERBLint::Cache::CACHE_DIRECTORY } let(:rubocop_yml) { %(SpaceAroundErbTag:\n Enabled: true\n) } let(:cache_file_content) do From be113700428906b82974c02a50c42a342c87f397 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Thu, 18 Aug 2022 20:19:23 -0400 Subject: [PATCH 10/85] Fix nonsensical error message --- lib/erb_lint/cli.rb | 2 +- spec/erb_lint/cli_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index d811f9a9..1644e712 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -45,7 +45,7 @@ def run(args = ARGV) cache.clear success!("cache directory cleared") else - failure!("cache directory already doesn't exist, skipped deletion.") + failure!("cache directory doesn't exist, skipping deletion.") end end diff --git a/spec/erb_lint/cli_spec.rb b/spec/erb_lint/cli_spec.rb index d282b131..25395dcc 100644 --- a/spec/erb_lint/cli_spec.rb +++ b/spec/erb_lint/cli_spec.rb @@ -101,7 +101,7 @@ def run(processed_source) context "with --clear-cache" do let(:args) { ["--clear-cache"] } context "without a cache folder" do - it { expect { subject }.to(output(/cache directory already doesn't exist, skipped deletion/).to_stderr) } + it { expect { subject }.to(output(/cache directory doesn't exist, skipping deletion/).to_stderr) } it "shows cache not cleared message if cache is empty and fails" do expect(subject).to(be(false)) end From deeb4f80efd8d7a33d33bbdadeee3c9376d77579 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Thu, 18 Aug 2022 20:31:46 -0400 Subject: [PATCH 11/85] Spacing fixes and removing unneeded code --- lib/erb_lint/cli.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 1644e712..016c370d 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -40,6 +40,7 @@ def run(args = ARGV) load_config @cache = Cache.new(@config, file_loader) if with_cache? || clear_cache? + if clear_cache? if cache.cache_dir_exists? cache.clear @@ -120,17 +121,19 @@ def run(args = ARGV) def run_on_file(runner, filename) file_content = read_content(filename) + 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) file_content end def run_using_cache(runner, filename, file_content) - if cache.include?(filename) && !autocorrect? + if cache.include?(filename) runner.restore_offenses(cache.get(filename, file_content)) cache.add_hit(filename) if prune_cache? else From 1a5db058f820ec6c40bbcda0e423aff0d42a86c0 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Fri, 19 Aug 2022 22:11:12 -0400 Subject: [PATCH 12/85] Fix tests by mocking returned checksum --- spec/erb_lint/cache_spec.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/erb_lint/cache_spec.rb b/spec/erb_lint/cache_spec.rb index ce7dde19..5651cf20 100644 --- a/spec/erb_lint/cache_spec.rb +++ b/spec/erb_lint/cache_spec.rb @@ -42,8 +42,11 @@ f.write(cache_file_content) end - allow(::RuboCop::ConfigLoader).to(receive(:load_file).and_call_original) - allow(::RuboCop::ConfigLoader).to(receive(:read_file).and_return(rubocop_yml)) + digest_sha1_double = instance_double(Digest::SHA1) + allow(Digest::SHA1).to(receive(:new).and_return(digest_sha1_double)) + allow(digest_sha1_double).to(receive(:hexdigest).and_return(checksum)) + allow(digest_sha1_double).to(receive(:update).and_return(true)) + allow(digest_sha1_double).to(receive(:file).and_return(true)) end describe "#get" do @@ -127,7 +130,7 @@ )).to(be(true)) end - it "prunes outdated cache results" do + it "prunes unused cache results" do fakefs_dir = Struct.new(:fakefs_dir) allow(fakefs_dir).to(receive(:children).and_return([checksum, "fake-checksum"])) allow(FakeFS::Dir).to(receive(:new).and_return(fakefs_dir)) From 8d4eaa86662ba9c34bf90259168cdda7a83660a1 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Sat, 8 Oct 2022 14:07:56 -0400 Subject: [PATCH 13/85] Move cache pruning to the cache class --- lib/erb_lint/cache.rb | 24 +++++++++++++++--------- lib/erb_lint/cli.rb | 6 ++---- spec/erb_lint/cache_spec.rb | 34 ++++++++++++++++++++++------------ 3 files changed, 39 insertions(+), 25 deletions(-) diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index a2760e0d..425cc801 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -4,15 +4,18 @@ module ERBLint class Cache CACHE_DIRECTORY = ".erb-lint-cache" - def initialize(config, file_loader = nil) + def initialize(config, file_loader = nil, prune = false) @config = config @file_loader = file_loader @hits = [] @new_results = [] + @prune = prune puts "Cache mode is on" end def get(filename, file_content) + @hits.push(filename) if prune? + JSON.parse(File.read(File.join(CACHE_DIRECTORY, checksum(filename)))).map do |offense| ERBLint::Offense.from_json(offense, config, @file_loader, file_content) end @@ -23,6 +26,8 @@ def include?(filename) end def []=(filename, offenses_as_json) + @new_results.push(filename) if prune? + FileUtils.mkdir_p(CACHE_DIRECTORY) File.open(File.join(CACHE_DIRECTORY, checksum(filename)), "wb") do |f| @@ -30,15 +35,12 @@ def []=(filename, offenses_as_json) end end - def add_hit(hit) - @hits.push(hit) - end - - def add_new_result(filename) - @new_results.push(filename) + def close + prune_cache if prune? end - def prune + 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 @@ -51,7 +53,7 @@ def prune next if hits_as_checksums.include?(cache_file) if new_results_as_checksums.include?(cache_file) - puts "Skipping deletion of new cache result #{cache_file} in prune" + puts "Skipping deletion of new cache result #{cache_file}" next end @@ -91,5 +93,9 @@ def checksum(file) # here. "_" end + + def prune? + @prune + end end end diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 016c370d..272bf2ad 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -39,7 +39,7 @@ def run(args = ARGV) load_config - @cache = Cache.new(@config, file_loader) if with_cache? || clear_cache? + @cache = Cache.new(@config, file_loader, prune_cache?) if with_cache? || clear_cache? if clear_cache? if cache.cache_dir_exists? @@ -93,7 +93,7 @@ def run(args = ARGV) end end - cache.prune if prune_cache? + cache.close if with_cache? || clear_cache? reporter.show @@ -135,11 +135,9 @@ def run_on_file(runner, filename) def run_using_cache(runner, filename, file_content) if cache.include?(filename) runner.restore_offenses(cache.get(filename, file_content)) - cache.add_hit(filename) if prune_cache? else run_with_corrections(runner, filename, file_content) cache[filename] = runner.offenses.map(&:to_json_format).to_json - cache.add_new_result(filename) if prune_cache? end end diff --git a/spec/erb_lint/cache_spec.rb b/spec/erb_lint/cache_spec.rb index 5651cf20..b30f238a 100644 --- a/spec/erb_lint/cache_spec.rb +++ b/spec/erb_lint/cache_spec.rb @@ -95,15 +95,15 @@ end end - describe "#prune" do + describe "#prune_cache" do it "skips prune if no cache hits" do allow(cache).to(receive(:hits).and_return([])) - expect { cache.prune }.to(output(/Cache being created for the first time, skipping prune/).to_stdout) + expect { cache.prune_cache }.to(output(/Cache being created for the first time, skipping prune/).to_stdout) end it "does not prune actual cache hits" do - cache.prune + cache.prune_cache expect(File.exist?( File.join( @@ -120,7 +120,7 @@ allow(fakefs_dir).to(receive(:children).and_return([checksum])) allow(FakeFS::Dir).to(receive(:new).and_return(fakefs_dir)) - expect { cache.prune }.to(output(/Skipping deletion of new cache result #{checksum} in prune/).to_stdout) + expect { cache.prune_cache }.to(output(/Skipping deletion of new cache result #{checksum}/).to_stdout) expect(File.exist?( File.join( @@ -140,7 +140,7 @@ f.write(cache_file_content) end - expect { cache.prune }.to(output(/Cleaning deleted cached file with checksum fake-checksum/).to_stdout) + expect { cache.prune_cache }.to(output(/Cleaning deleted cached file with checksum fake-checksum/).to_stdout) expect(File.exist?( File.join( @@ -151,19 +151,29 @@ end end - describe "#add_new_result" do - it "adds new result to cache object new_results attribute" do - cache.add_new_result(linted_file_path) + describe "prune cache mode on #get and #[] behavior" do + before do + allow(cache).to(receive(:prune?).and_return(true)) + end + + it "adds new result to cache object new_results list attribute" do + cache[linted_file_path] = cache_file_content expect(cache.send(:new_results)).to(include(linted_file_path)) end - end - describe "#add_hit" do - it "adds new cache hit to cache object hits attribute" do - cache.add_hit(linted_file_path) + it "adds new cache hit to cache object hits list attribute" do + cache.get(linted_file_path, linted_file_content) expect(cache.send(:hits)).to(include(linted_file_path)) end end + + describe "#close" do + it "Calls prune_cache if prune_cache mode is on" do + allow(cache).to(receive(:prune?).and_return(true)) + expect(cache).to(receive(:prune_cache)) + cache.close + end + end end From cdf089ccb2d92a8dbfc2cf179d3dcd9c7a70f5b9 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Sat, 8 Oct 2022 16:01:57 -0400 Subject: [PATCH 14/85] Do less file reads and stats --- lib/erb_lint/cache.rb | 39 +++++++++++++++++++------------------ lib/erb_lint/cli.rb | 6 +++--- spec/erb_lint/cache_spec.rb | 24 +++++++---------------- 3 files changed, 30 insertions(+), 39 deletions(-) diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 425cc801..ee8fb639 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -14,23 +14,27 @@ def initialize(config, file_loader = nil, prune = false) end def get(filename, file_content) - @hits.push(filename) if prune? - - JSON.parse(File.read(File.join(CACHE_DIRECTORY, checksum(filename)))).map do |offense| - ERBLint::Offense.from_json(offense, config, @file_loader, 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::Offense.from_json(offense, config, @file_loader, file_content) + end + rescue Errno::ENOENT + return false end + @hits.push(file_checksum) if prune? + cache_file_contents_as_offenses end - def include?(filename) - File.exist?(File.join(CACHE_DIRECTORY, checksum(filename))) - end - - def []=(filename, offenses_as_json) - @new_results.push(filename) if prune? + 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, checksum(filename)), "wb") do |f| + File.open(File.join(CACHE_DIRECTORY, file_checksum), "wb") do |f| f.write(offenses_as_json) end end @@ -47,12 +51,10 @@ def prune_cache end cache_files = Dir.new(CACHE_DIRECTORY).children - hits_as_checksums = hits.map { |hit| checksum(hit) } - new_results_as_checksums = new_results.map { |new_result| checksum(new_result) } cache_files.each do |cache_file| - next if hits_as_checksums.include?(cache_file) + next if hits.include?(cache_file) - if new_results_as_checksums.include?(cache_file) + if new_results.include?(cache_file) puts "Skipping deletion of new cache result #{cache_file}" next end @@ -79,14 +81,13 @@ def clear attr_reader :config, :hits, :new_results - def checksum(file) + def checksum(filename, file_content) digester = Digest::SHA1.new - mode = File.stat(file).mode + mode = File.stat(filename).mode digester.update( - "#{file}#{mode}#{config.to_hash}#{ERBLint::VERSION}" + "#{mode}#{config.to_hash}#{ERBLint::VERSION}#{file_content}" ) - digester.file(file) digester.hexdigest rescue Errno::ENOENT # Spurious files that come and go should not cause a crash, at least not diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 272bf2ad..7cedf1df 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -133,11 +133,11 @@ def run_on_file(runner, filename) end def run_using_cache(runner, filename, file_content) - if cache.include?(filename) - runner.restore_offenses(cache.get(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[filename] = runner.offenses.map(&:to_json_format).to_json + cache.set(filename, file_content, runner.offenses.map(&:to_json_format).to_json) end end diff --git a/spec/erb_lint/cache_spec.rb b/spec/erb_lint/cache_spec.rb index b30f238a..141ccb75 100644 --- a/spec/erb_lint/cache_spec.rb +++ b/spec/erb_lint/cache_spec.rb @@ -51,14 +51,14 @@ describe "#get" do it "returns a cached lint result" do - cache_result = cache.get(linted_file_path, cache_file_content) + cache_result = cache.get(linted_file_path, linted_file_content) expect(cache_result.count).to(eq(2)) end end describe "#[]=" do it "caches a lint result" do - cache[linted_file_path] = cache_file_content + cache.set(linted_file_path, linted_file_content, cache_file_content) expect(File.exist?( File.join( cache_dir, @@ -68,16 +68,6 @@ end end - describe "#include?" do - it "returns true if the cache includes the filename" do - expect(cache.include?(linted_file_path)).to(be(true)) - end - - it "returns false if the cache does not include the filename" do - expect(cache.include?("gibberish")).to(be(false)) - end - end - describe "#cache_dir_exists?" do it "returns true if the cache dir exists" do expect(cache.cache_dir_exists?).to(be(true)) @@ -115,12 +105,12 @@ it "does not prune new cache results" do allow(cache).to(receive(:hits).and_return(["fake-hit"])) - allow(cache).to(receive(:new_results).and_return([linted_file_path])) + allow(cache).to(receive(:new_results).and_return([checksum])) fakefs_dir = Struct.new(:fakefs_dir) allow(fakefs_dir).to(receive(:children).and_return([checksum])) allow(FakeFS::Dir).to(receive(:new).and_return(fakefs_dir)) - expect { cache.prune_cache }.to(output(/Skipping deletion of new cache result #{checksum}/).to_stdout) + expect { cache.prune_cache }.to(output(/.*Skipping deletion of new cache result #{checksum}/).to_stdout) expect(File.exist?( File.join( @@ -157,15 +147,15 @@ end it "adds new result to cache object new_results list attribute" do - cache[linted_file_path] = cache_file_content + cache.set(linted_file_path, linted_file_content, cache_file_content) - expect(cache.send(:new_results)).to(include(linted_file_path)) + expect(cache.send(:new_results)).to(include(checksum)) end it "adds new cache hit to cache object hits list attribute" do cache.get(linted_file_path, linted_file_content) - expect(cache.send(:hits)).to(include(linted_file_path)) + expect(cache.send(:hits)).to(include(checksum)) end end From b331799a1a0543d29cdbf33a94a7a7da871a90c3 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Mon, 10 Oct 2022 15:48:52 -0400 Subject: [PATCH 15/85] Attempt a new implementation for storing offenses in a CachedOffense for speed --- lib/erb_lint/all.rb | 1 + lib/erb_lint/cache.rb | 2 +- lib/erb_lint/cached_offense.rb | 39 ++++++++++++++++++++ lib/erb_lint/cli.rb | 2 +- lib/erb_lint/offense.rb | 45 +---------------------- spec/erb_lint/fixtures/cache_file_content | 2 +- 6 files changed, 45 insertions(+), 46 deletions(-) create mode 100644 lib/erb_lint/cached_offense.rb diff --git a/lib/erb_lint/all.rb b/lib/erb_lint/all.rb index 8280343a..4aee601c 100644 --- a/lib/erb_lint/all.rb +++ b/lib/erb_lint/all.rb @@ -4,6 +4,7 @@ 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" diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index ee8fb639..73d1f42c 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -19,7 +19,7 @@ def get(filename, file_content) cache_file_contents_as_offenses = JSON.parse( File.read(File.join(CACHE_DIRECTORY, file_checksum)) ).map do |offense| - ERBLint::Offense.from_json(offense, config, @file_loader, file_content) + ERBLint::CachedOffense.from_json(offense) end rescue Errno::ENOENT return false diff --git a/lib/erb_lint/cached_offense.rb b/lib/erb_lint/cached_offense.rb new file mode 100644 index 00000000..7710d332 --- /dev/null +++ b/lib/erb_lint/cached_offense.rb @@ -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 + ) + end + end +end diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 7cedf1df..d317c048 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -137,7 +137,7 @@ def run_using_cache(runner, 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_json_format).to_json) + cache.set(filename, file_content, runner.offenses.map(&:to_cached_offense_json_format).to_json) end end diff --git a/lib/erb_lint/offense.rb b/lib/erb_lint/offense.rb index 8c306239..9e76bcb5 100644 --- a/lib/erb_lint/offense.rb +++ b/lib/erb_lint/offense.rb @@ -17,49 +17,8 @@ def initialize(linter, source_range, message, context = nil, severity = nil) @severity = severity end - def to_json_format - { - linter_config: linter.config.to_hash, - linter_config_class: linter.config.class.name, - linter_class: linter.class.name, - source_range: source_range_to_json_format, - message: message, - context: context, - severity: severity, - } - end - - def source_range_to_json_format - { - begin_pos: source_range.begin_pos, - end_pos: source_range.end_pos, - source_buffer_name: source_range.source_buffer.name, - } - end - - def self.from_json(parsed_json, config, file_loader, file_content) - parsed_json.transform_keys!(&:to_sym) - linter_config = const_get(parsed_json[:linter_config_class]).new(parsed_json[:linter_config]) - linter = const_get(parsed_json[:linter_class]).new(file_loader, linter_config) - new( - linter, - source_range_from_json_format(parsed_json[:source_range], file_content), - parsed_json[:message].presence, - parsed_json[:context].presence, - parsed_json[:severity].presence&.to_sym - ) - end - - def self.source_range_from_json_format(parsed_json_source_range, file_content) - parsed_json_source_range.transform_keys!(&:to_sym) - Parser::Source::Range.new( - Parser::Source::Buffer.new( - parsed_json_source_range[:source_buffer_name], - source: file_content - ), - parsed_json_source_range[:begin_pos], - parsed_json_source_range[:end_pos] - ) + def to_cached_offense_json_format + ERBLint::CachedOffense.new_from_offense(self).to_json_format end def inspect diff --git a/spec/erb_lint/fixtures/cache_file_content b/spec/erb_lint/fixtures/cache_file_content index e044f385..62945f92 100644 --- a/spec/erb_lint/fixtures/cache_file_content +++ b/spec/erb_lint/fixtures/cache_file_content @@ -1 +1 @@ -[{"linter_config":{"enabled":true,"exclude":[]},"linter_config_class":"ERBLint::LinterConfig","linter_class":"ERBLint::Linters::SpaceAroundErbTag","source_range":{"begin_pos":2,"end_pos":2,"source_buffer_name":"/home/zach/dev/fake-app/app/components/elements/image_component/image_component.html.erb"},"message":"Use 1 space after `<%` instead of 0 space.","context":" ","severity":null},{"linter_config":{"enabled":true,"exclude":[]},"linter_config_class":"ERBLint::LinterConfig","linter_class":"ERBLint::Linters::SpaceAroundErbTag","source_range":{"begin_pos":172,"end_pos":172,"source_buffer_name":"/home/zach/dev/fake-app/app/components/elements/image_component/image_component.html.erb"},"message":"Use 1 space before `%>` instead of 0 space.","context":" ","severity":null}] \ No newline at end of file +[{"message":"Layout/InitialIndentation: Indentation of first line in file detected.","line_number":"1","severity":"convention"},{"message":"Layout/TrailingEmptyLines: Final newline missing.","line_number":"1","severity":"convention"}] \ No newline at end of file From ce8d6da6d4d51cc460930bc905564485fd1f357a Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Tue, 11 Oct 2022 21:32:40 -0400 Subject: [PATCH 16/85] Update lib/erb_lint/cached_offense.rb to avoid errors on nil severity Co-authored-by: Eric Terry --- lib/erb_lint/cached_offense.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/erb_lint/cached_offense.rb b/lib/erb_lint/cached_offense.rb index 7710d332..4a726934 100644 --- a/lib/erb_lint/cached_offense.rb +++ b/lib/erb_lint/cached_offense.rb @@ -32,7 +32,7 @@ def self.from_json(parsed_json) new( parsed_json[:message], parsed_json[:line_number], - parsed_json[:severity].to_sym + parsed_json[:severity]&.to_sym ) end end From acba4a9011cf3e2e0479af0f77804030a14dd163 Mon Sep 17 00:00:00 2001 From: Eric Terry Date: Tue, 11 Oct 2022 23:56:52 -0500 Subject: [PATCH 17/85] Support caching with json and compact reporters --- lib/erb_lint/cache.rb | 4 +- lib/erb_lint/cached_offense.rb | 55 +++++++++++++------ lib/erb_lint/cli.rb | 2 +- lib/erb_lint/offense.rb | 20 ++++++- lib/erb_lint/reporters/json_reporter.rb | 8 +-- spec/erb_lint/fixtures/cache_file_content | 2 +- spec/erb_lint/reporters/json_reporter_spec.rb | 22 +++----- 7 files changed, 71 insertions(+), 42 deletions(-) diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 73d1f42c..79b88a81 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -18,8 +18,8 @@ def get(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) + ).map do |offense_hash| + ERBLint::CachedOffense.new(offense_hash) end rescue Errno::ENOENT return false diff --git a/lib/erb_lint/cached_offense.rb b/lib/erb_lint/cached_offense.rb index 4a726934..3c5fa74e 100644 --- a/lib/erb_lint/cached_offense.rb +++ b/lib/erb_lint/cached_offense.rb @@ -3,37 +3,56 @@ module ERBLint # A Cached version of an Offense with only essential information represented as strings class CachedOffense - attr_reader :line_number, :message, :severity + attr_reader( + :message, + :line_number, + :severity, + :column, + :simple_name, + :last_line, + :last_column, + :length, + ) - def initialize(message, line_number, severity) - @message = message - @line_number = line_number - @severity = severity + def initialize(params) + params = params.transform_keys(&:to_sym) + + @message = params[:message] + @line_number = params[:line_number] + @severity = params[:severity]&.to_sym + @column = params[:column] + @simple_name = params[:simple_name] + @last_line = params[:last_line] + @last_column = params[:last_column] + @length = params[:length] end def self.new_from_offense(offense) new( - offense.message, - offense.line_number.to_s, - offense.severity + { + message: offense.message, + line_number: offense.line_number, + severity: offense.severity, + column: offense.column, + simple_name: offense.simple_name, + last_line: offense.last_line, + last_column: offense.last_column, + length: offense.length, + } ) end - def to_json_format + def to_h { message: message, line_number: line_number, severity: severity, + column: column, + simple_name: simple_name, + last_line: last_line, + last_column: last_column, + length: length, } 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 - ) - end end end diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index d317c048..72b8c131 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -137,7 +137,7 @@ def run_using_cache(runner, 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) + cache.set(filename, file_content, runner.offenses.map(&:to_cached_offense_hash).to_json) end end diff --git a/lib/erb_lint/offense.rb b/lib/erb_lint/offense.rb index 9e76bcb5..5c8db6b7 100644 --- a/lib/erb_lint/offense.rb +++ b/lib/erb_lint/offense.rb @@ -17,8 +17,8 @@ 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 + def to_cached_offense_hash + ERBLint::CachedOffense.new_from_offense(self).to_h end def inspect @@ -47,5 +47,21 @@ def line_number def column source_range.column end + + def simple_name + linter.class.simple_name + end + + def last_line + source_range.last_line + end + + def last_column + source_range.last_column + end + + def length + source_range.length + end end end diff --git a/lib/erb_lint/reporters/json_reporter.rb b/lib/erb_lint/reporters/json_reporter.rb index c3f937f2..2abf35e7 100644 --- a/lib/erb_lint/reporters/json_reporter.rb +++ b/lib/erb_lint/reporters/json_reporter.rb @@ -56,14 +56,14 @@ def formatted_offenses(offenses) def format_offense(offense) { - linter: offense.linter.class.simple_name, + linter: offense.simple_name, message: offense.message.to_s, location: { start_line: offense.line_number, start_column: offense.column, - last_line: offense.source_range.last_line, - last_column: offense.source_range.last_column, - length: offense.source_range.length, + last_line: offense.last_line, + last_column: offense.last_column, + length: offense.length, }, } end diff --git a/spec/erb_lint/fixtures/cache_file_content b/spec/erb_lint/fixtures/cache_file_content index 62945f92..20d5f8d6 100644 --- a/spec/erb_lint/fixtures/cache_file_content +++ b/spec/erb_lint/fixtures/cache_file_content @@ -1 +1 @@ -[{"message":"Layout/InitialIndentation: Indentation of first line in file detected.","line_number":"1","severity":"convention"},{"message":"Layout/TrailingEmptyLines: Final newline missing.","line_number":"1","severity":"convention"}] \ No newline at end of file +[{"message":"Layout/InitialIndentation: Indentation of first line in file detected.","line_number":"1","severity":"convention","line_number":"1","severity":"convention","column":"3","simple_name":"Rubocop","last_line":"1","last_column":"8","length":"5"},{"message":"Layout/TrailingEmptyLines: Final newline missing.","line_number":"1","severity":"convention","line_number":"1","severity":"convention","column":"3","simple_name":"Rubocop","last_line":"1","last_column":"8","length":"5"}] diff --git a/spec/erb_lint/reporters/json_reporter_spec.rb b/spec/erb_lint/reporters/json_reporter_spec.rb index 9119ab0f..d5b62d0b 100644 --- a/spec/erb_lint/reporters/json_reporter_spec.rb +++ b/spec/erb_lint/reporters/json_reporter_spec.rb @@ -23,26 +23,20 @@ message: "Extra space detected where there should be no space.", line_number: 1, column: 7, - source_range: instance_double( - BetterHtml::Tokenizer::Location, - last_line: 1, - last_column: 9, - length: 2, - ), - linter: ERBLint::Linters::SpaceInHtmlTag.new(nil, ERBLint::LinterConfig.new), + simple_name: "SpaceInHtmlTag", + last_line: 1, + last_column: 9, + length: 2, ), instance_double( ERBLint::Offense, message: "Remove newline before `%>` to match start of tag.", line_number: 52, column: 10, - source_range: instance_double( - BetterHtml::Tokenizer::Location, - last_line: 54, - last_column: 10, - length: 10, - ), - linter: ERBLint::Linters::ClosingErbTagIndent.new(nil, ERBLint::LinterConfig.new), + simple_name: "ClosingErbTagIndent", + last_line: 54, + last_column: 10, + length: 10, ), ] end From 9ec34e399cfd366d5fa6fec2214715a7e57fc97f Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Thu, 13 Oct 2022 07:41:30 -0400 Subject: [PATCH 18/85] Update lib/erb_lint/cli.rb to use safe navigation operator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Étienne Barrié --- lib/erb_lint/cli.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 72b8c131..576b6df0 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -93,7 +93,7 @@ def run(args = ARGV) end end - cache.close if with_cache? || clear_cache? + cache&.close reporter.show From 877c28cfedb14d0cd51ac109aaa3ae6158ee9542 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Thu, 13 Oct 2022 07:44:34 -0400 Subject: [PATCH 19/85] Rename with-cache to cache --- lib/erb_lint/cli.rb | 14 +++++++------- spec/erb_lint/cli_spec.rb | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 576b6df0..ae11f391 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -31,7 +31,7 @@ def run(args = ARGV) dupped_args = args.dup load_options(dupped_args) - if with_cache? && autocorrect? + if cache? && autocorrect? failure!("cannot run autocorrect mode with cache") end @@ -39,7 +39,7 @@ def run(args = ARGV) load_config - @cache = Cache.new(@config, file_loader, prune_cache?) if with_cache? || clear_cache? + @cache = Cache.new(@config, file_loader, prune_cache?) if cache? || clear_cache? if clear_cache? if cache.cache_dir_exists? @@ -122,7 +122,7 @@ def run(args = ARGV) def run_on_file(runner, filename) file_content = read_content(filename) - if with_cache? && !autocorrect? + if cache? && !autocorrect? run_using_cache(runner, filename, file_content) else file_content = run_with_corrections(runner, filename, file_content) @@ -145,8 +145,8 @@ def autocorrect? @options[:autocorrect] end - def with_cache? - @options[:with_cache] + def cache? + @options[:cache] end def prune_cache? @@ -338,8 +338,8 @@ def option_parser @options[:enabled_linters] = known_linter_names end - opts.on("--with-cache", "Enable caching") do |config| - @options[:with_cache] = config + opts.on("--cache", "Enable caching") do |config| + @options[:cache] = config end opts.on("--prune-cache", "Prune cache") do |config| diff --git a/spec/erb_lint/cli_spec.rb b/spec/erb_lint/cli_spec.rb index 25395dcc..a1a23d38 100644 --- a/spec/erb_lint/cli_spec.rb +++ b/spec/erb_lint/cli_spec.rb @@ -249,7 +249,7 @@ def run(processed_source) end context "with cache" do - let(:args) { ["--enable-linter", "linter_without_errors", "--with-cache", linted_file] } + let(:args) { ["--enable-linter", "linter_without_errors", "--cache", linted_file] } it "lints the file and adds it to the cache" do expect(Dir[ERBLint::Cache::CACHE_DIRECTORY].length).to(be(0)) @@ -472,7 +472,7 @@ def run(processed_source) end context "with cache" do - let(:args) { ["--enable-linter", "linter_without_errors", "--with-cache", linted_dir] } + let(:args) { ["--enable-linter", "linter_without_errors", "--cache", linted_dir] } it "lints the file and adds it to the cache" do expect(Dir[ERBLint::Cache::CACHE_DIRECTORY].length).to(be(0)) @@ -590,7 +590,7 @@ def run(processed_source) context "when autocorrecting and caching are turned on" do # We assume that linter_with_errors is not autocorrectable... let(:args) do - ["--enable-linter", "linter_without_errors", "--stdin", linted_file, "--autocorrect", "--with-cache"] + ["--enable-linter", "linter_without_errors", "--stdin", linted_file, "--autocorrect", "--cache"] end it "throws an error saying the two modes cannot be used together" do From 7ea048c5960d750d5a3b1bfe292436ecfe13436c Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Thu, 13 Oct 2022 07:55:51 -0400 Subject: [PATCH 20/85] Prune cache by default --- lib/erb_lint/cache.rb | 15 +++++---------- lib/erb_lint/cli.rb | 10 +--------- spec/erb_lint/cache_spec.rb | 21 ++------------------- 3 files changed, 8 insertions(+), 38 deletions(-) diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 79b88a81..9efcd6a5 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -4,12 +4,11 @@ module ERBLint class Cache CACHE_DIRECTORY = ".erb-lint-cache" - def initialize(config, file_loader = nil, prune = false) + def initialize(config, file_loader = nil) @config = config @file_loader = file_loader @hits = [] @new_results = [] - @prune = prune puts "Cache mode is on" end @@ -24,13 +23,13 @@ def get(filename, file_content) rescue Errno::ENOENT return false end - @hits.push(file_checksum) if prune? + @hits.push(file_checksum) 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? + @new_results.push(file_checksum) FileUtils.mkdir_p(CACHE_DIRECTORY) @@ -40,11 +39,11 @@ def set(filename, file_content, offenses_as_json) end def close - prune_cache if prune? + prune_cache end def prune_cache - puts "Prune cache mode is on - pruned file names will be logged" + puts "File names pruned from the cache will be logged" if hits.empty? puts "Cache being created for the first time, skipping prune" return @@ -94,9 +93,5 @@ def checksum(filename, file_content) # here. "_" end - - def prune? - @prune - end end end diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index ae11f391..e21e4c4f 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -39,7 +39,7 @@ def run(args = ARGV) load_config - @cache = Cache.new(@config, file_loader, prune_cache?) if cache? || clear_cache? + @cache = Cache.new(@config, file_loader) if cache? || clear_cache? if clear_cache? if cache.cache_dir_exists? @@ -149,10 +149,6 @@ def cache? @options[:cache] end - def prune_cache? - @options[:prune_cache] - end - def clear_cache? @options[:clear_cache] end @@ -342,10 +338,6 @@ def option_parser @options[: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 diff --git a/spec/erb_lint/cache_spec.rb b/spec/erb_lint/cache_spec.rb index 141ccb75..d6d6689a 100644 --- a/spec/erb_lint/cache_spec.rb +++ b/spec/erb_lint/cache_spec.rb @@ -53,6 +53,7 @@ it "returns a cached lint result" do cache_result = cache.get(linted_file_path, linted_file_content) expect(cache_result.count).to(eq(2)) + expect(cache.send(:hits)).to(include(checksum)) end end @@ -65,6 +66,7 @@ checksum ) )).to(be(true)) + expect(cache.send(:new_results)).to(include(checksum)) end end @@ -141,27 +143,8 @@ end end - describe "prune cache mode on #get and #[] behavior" do - before do - allow(cache).to(receive(:prune?).and_return(true)) - end - - it "adds new result to cache object new_results list attribute" do - cache.set(linted_file_path, linted_file_content, cache_file_content) - - expect(cache.send(:new_results)).to(include(checksum)) - end - - it "adds new cache hit to cache object hits list attribute" do - cache.get(linted_file_path, linted_file_content) - - expect(cache.send(:hits)).to(include(checksum)) - end - end - describe "#close" do it "Calls prune_cache if prune_cache mode is on" do - allow(cache).to(receive(:prune?).and_return(true)) expect(cache).to(receive(:prune_cache)) cache.close end From a32816d29817bdd056b9fd3ca487d58db0467241 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Thu, 13 Oct 2022 08:00:06 -0400 Subject: [PATCH 21/85] Add README for cache --- README.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/README.md b/README.md index 6c026dba..9d0acdb6 100644 --- a/README.md +++ b/README.md @@ -590,6 +590,23 @@ app/views/users/_graph.html.erb:27:37: Extra space detected where there should b 2 error(s) were found in ERB files ``` +## Caching + +The cache is currently opt-in - to turn it on, use the --cache option: + +```sh +erblint --cache ./app +Cache mode is on +Linting 413 files with 15 linters... +File names pruned from the cache will be logged + +No errors were found in ERB files +``` + +When the cache is on, lint results are stored in the `.erb-lint-cache` directory, in files with a filename computed with a hash of information about the file and `erb-lint` that should change when necessary. These files store instance attributes of the `CachedOffense` object, which only contain the `Offense` attributes necessary to restore the results of running `erb-lint` for output. The cache also automatically prunes outdated files each time it's run. + +You can also use the --clear-cache option to delete the cache file directory. + ## License This project is released under the [MIT license](LICENSE.txt). From e08285533ad14d9ffcfc17ae1475f7d416647452 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Tue, 18 Oct 2022 08:14:20 -0400 Subject: [PATCH 22/85] Reduce output of pruning process --- lib/erb_lint/cache.rb | 11 +---------- spec/erb_lint/cache_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 9efcd6a5..ee537e17 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -43,7 +43,6 @@ def close end def prune_cache - puts "File names pruned from the cache will be logged" if hits.empty? puts "Cache being created for the first time, skipping prune" return @@ -51,18 +50,10 @@ def prune_cache cache_files = Dir.new(CACHE_DIRECTORY).children cache_files.each do |cache_file| - next if hits.include?(cache_file) + next if hits.include?(cache_file) || new_results.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? diff --git a/spec/erb_lint/cache_spec.rb b/spec/erb_lint/cache_spec.rb index d6d6689a..93731202 100644 --- a/spec/erb_lint/cache_spec.rb +++ b/spec/erb_lint/cache_spec.rb @@ -112,7 +112,7 @@ allow(fakefs_dir).to(receive(:children).and_return([checksum])) allow(FakeFS::Dir).to(receive(:new).and_return(fakefs_dir)) - expect { cache.prune_cache }.to(output(/.*Skipping deletion of new cache result #{checksum}/).to_stdout) + cache.prune_cache expect(File.exist?( File.join( @@ -132,7 +132,7 @@ f.write(cache_file_content) end - expect { cache.prune_cache }.to(output(/Cleaning deleted cached file with checksum fake-checksum/).to_stdout) + cache.prune_cache expect(File.exist?( File.join( From 5f9092960f1465f11bfeda3f4a8897b3baf6a9ed Mon Sep 17 00:00:00 2001 From: Issy Long Date: Wed, 19 Oct 2022 14:42:40 +0100 Subject: [PATCH 23/85] lib/erb_lint/linters: Add a new `CommentSyntax` linter - This adds a new ERB comment syntax linter to catch ERB comments that could trip up the Ruby parser in latest versions of Ruby. - The ERB documentation[1] says that Ruby comments are not valid in ERB, and that comments in ERB should be of the form `<%# comment here %>` and not `<% # comment here %>`. That is, no space between the opening `<%` and the `#` character. - This documentation is only a guideline usually, because in most cases Ruby-style comments work just fine. However, using the Ruby 3.1 parser, RuboCop's `Lint/Syntax` cop started failing on ERB files that included comments with semicolons in them. - Take an example ERB file: ``` <%# This is the correct comment syntax. %> <%# Recommending the "proper" ERB comment syntax seems like the safest way to go; even with semicolons it will parse. %> <% # This is technically incorrect comment syntax but it generally parses. %> <% # Until someone puts a ; in. %> ``` - This led to output from RuboCop's `Lint/Syntax` cop (RuboCop v1.36, `TargetRubyVersion: 3.1`) where it failed to parse the file, but its code snippets and offense locations were not very useful. (Admittedly it's less visible in this contrived example, you'll have to trust me!) ``` app/views/proof_of_concept.html.erb:7:2: E: Lint/Syntax: unexpected token kIN (Using Ruby 3.1 parser; configure using TargetRubyVersion parameter, under AllCops) in. ^^ 1 file inspected, 1 offense detected ``` - After lots of being confused, because the code looked like valid Ruby and the editor was syntax highlighting it if it were a valid comment, we realised that it was the semicolon in the comment in the ERB that was tripping the Ruby 3.1 parser up. - Doing some digging, it turns out that this could have all been avoided if our codebase had respected ERB's "Ruby comments are not valid" guideline[1]. I went hunting for a RuboCop rule for this, then remembered that ERBLint existed and is way better suited to linting ERB. There wasn't an existing rule for ERB comment syntax that I could find, hence this addition, because I feel like individual developers shouldn't have to know this quirk of ERB off the top of their heads when it can cause such confusing errors! [1] - https://github.com/ruby/erb/tree/a3492c4bd1061071814cca085544ce259a9d8d56#recognized-tags --- lib/erb_lint/linters/comment_syntax.rb | 64 ++++++++++++++++++++ spec/erb_lint/linters/comment_syntax_spec.rb | 40 ++++++++++++ 2 files changed, 104 insertions(+) create mode 100644 lib/erb_lint/linters/comment_syntax.rb create mode 100644 spec/erb_lint/linters/comment_syntax_spec.rb diff --git a/lib/erb_lint/linters/comment_syntax.rb b/lib/erb_lint/linters/comment_syntax.rb new file mode 100644 index 00000000..214e90ef --- /dev/null +++ b/lib/erb_lint/linters/comment_syntax.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +module ERBLint + module Linters + # Detects comment syntax that isn't valid ERB. + class CommentSyntax < Linter + include LinterRegistry + + def initialize(file_loader, config) + super + end + + def run(processed_source) + file_content = processed_source.file_content + + return if file_content.empty? + + processed_source.ast.descendants(:erb).each do |erb_node| + indicator_node, _, code_node, _ = *erb_node + next if indicator_node.nil? || code_node.nil? + + indicator_node_str = indicator_node.deconstruct.last + next if indicator_node_str == "#" + + if indicator_node_str == "=" + range = find_range(erb_node, indicator_node_str) + source_range = processed_source.to_source_range(range) + + add_offense( + source_range, + <<~EOF.chomp + Bad ERB comment syntax. Should be `<%#=` without a space between. + Leaving a space between ERB tags and the Ruby comment character can cause parser errors. + EOF + ) + end + + code_node_str = code_node.deconstruct.last + next unless code_node_str.start_with?(" #") + + range = find_range(erb_node, code_node_str) + source_range = processed_source.to_source_range(range) + + add_offense( + source_range, + <<~EOF.chomp + Bad ERB comment syntax. Should be `<%#` without a space between. + Leaving a space between ERB tags and the Ruby comment character can cause parser errors. + EOF + ) + end + end + + def find_range(node, str) + match = node.loc.source.match(Regexp.new(Regexp.quote(str.strip))) + return unless match + + range_begin = match.begin(0) + node.loc.begin_pos + range_end = match.end(0) + node.loc.begin_pos + (range_begin...range_end) + end + end + end +end diff --git a/spec/erb_lint/linters/comment_syntax_spec.rb b/spec/erb_lint/linters/comment_syntax_spec.rb new file mode 100644 index 00000000..b21d7aae --- /dev/null +++ b/spec/erb_lint/linters/comment_syntax_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe ERBLint::Linters::CommentSyntax do + let(:linter_config) { described_class.config_schema.new } + + let(:file_loader) { ERBLint::FileLoader.new(".") } + let(:linter) { described_class.new(file_loader, linter_config) } + let(:processed_source) { ERBLint::ProcessedSource.new("file.rb", file) } + + subject { linter.offenses } + before { linter.run(processed_source) } + + context "when the ERB comment syntax is correct" do + let(:file) { <<~FILE } + <%# good comment %> + FILE + + it "does not report any offenses" do + expect(subject.size).to(eq(0)) + end + end + + context "when the ERB comment syntax is incorrect" do + let(:file) { <<~FILE } + <% # first bad comment %> + <%= # second bad comment %> + FILE + + it "reports two offenses" do + expect(subject.size).to(eq(2)) + end + + it "reports two offenses with their suggested fixes" do + expect(subject.first.message).to(include("Bad ERB comment syntax. Should be `<%#=` without a space between.")) + expect(subject.last.message).to(include("Bad ERB comment syntax. Should be `<%#` without a space between.")) + end + end +end From 911faf3c0868aafafdd66f07e3b2b13caed4b644 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Thu, 20 Oct 2022 10:18:58 +0100 Subject: [PATCH 24/85] linters/CommentSyntax: Improve docs & code style; add another test - It's better to have too many test cases than too few, and I realised forgot about the fact that multi-line ERB comments _can_ have spaces. --- README.md | 1 + lib/erb_lint/linters/comment_syntax.rb | 1 - spec/erb_lint/linters/comment_syntax_spec.rb | 12 ++++++++++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 6c026dba..23ff64ad 100644 --- a/README.md +++ b/README.md @@ -110,6 +110,7 @@ linters: | [ErbSafety](#ErbSafety) | No | detects unsafe interpolation of ruby data into various javascript contexts and enforce usage of safe helpers like `.to_json`. | | [Rubocop](#Rubocop) | No | runs RuboCop rules on ruby statements found in ERB templates | | [RequireScriptNonce](#RequireScriptNonce) | No | warns about missing [Content Security Policy nonces](https://guides.rubyonrails.org/security.html#content-security-policy) in script tags | +| CommentSyntax | No | detects bad comment syntax, ie `<% # comment %>` is not technically valid ERB but `<%# comment %>` is | ### DeprecatedClasses diff --git a/lib/erb_lint/linters/comment_syntax.rb b/lib/erb_lint/linters/comment_syntax.rb index 214e90ef..6a0aef9b 100644 --- a/lib/erb_lint/linters/comment_syntax.rb +++ b/lib/erb_lint/linters/comment_syntax.rb @@ -12,7 +12,6 @@ def initialize(file_loader, config) def run(processed_source) file_content = processed_source.file_content - return if file_content.empty? processed_source.ast.descendants(:erb).each do |erb_node| diff --git a/spec/erb_lint/linters/comment_syntax_spec.rb b/spec/erb_lint/linters/comment_syntax_spec.rb index b21d7aae..c098bd1e 100644 --- a/spec/erb_lint/linters/comment_syntax_spec.rb +++ b/spec/erb_lint/linters/comment_syntax_spec.rb @@ -22,6 +22,18 @@ end end + context "when the ERB multi-line comment syntax is correct" do + let(:file) { <<~FILE } + <% + # good comment + %> + FILE + + it "does not report any offenses" do + expect(subject.size).to(eq(0)) + end + end + context "when the ERB comment syntax is incorrect" do let(:file) { <<~FILE } <% # first bad comment %> From 4166d9bf883fcea2c01bb5e3599de173182bf3d2 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Thu, 20 Oct 2022 11:24:09 +0100 Subject: [PATCH 25/85] linters/CommentSyntax: Fix the `<% # comment %>` detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Previously I erroneously did a `next if indicator_node.nil?`, which meant that for a file with multiple offenses where one of them was `<%= # comment %>` which _was_ getting correctly detected, it was working, but the last one `<% # comment %>` syntax was not being detected properly. - This bug manifested in having two offenses detected for the same line, because it was never moving past the first detected offense because in the `<%` ERB opening case, `indicator_node` is nil and was getting skipped. Not what I originally intended! There were two results, however, and the tests passed. This was because for `<%= # bad comment here %>` it was detecting `<%=` for the `indicator_node_str == "#"` condition and then adding another offense because that same line's `code_node_str` (` # bad comment here`) started with ` #` so also matched the second condition which added another offense. (See examples below.) - This commit also refactors a bit to make things simpler and less repetitive - since `add_offense` appends to an array of offenses internally, we can only call it once and get rid of the multiple ifs because they're confusing, tweaking the message depending on whether it's a `<%=` expression or a `<%` expression. Sample ERB file: ``` <%# erb comment here %> <%= # bad erb comment here %> <% # apparently this comment syntax is valid? %> <% # very; bad erb comment here %> ``` Example behaviour before: ``` Bad ERB comment syntax. Should be <%#= without a space between. Leaving a space between ERB tags and the Ruby comment character can cause parser errors. In file: test_comment_syntax.html.erb:2 Bad ERB comment syntax. Should be <%# without a space between. Leaving a space between ERB tags and the Ruby comment character can cause parser errors. In file: test_comment_syntax.html.erb:2 ``` Example behaviour now (fixed): ``` ❯ exe/erblint --enable-linters=comment_syntax test_comment_syntax.html.erb .erb-lint.yml not found: using default config Linting 1 files with 1 linters... Bad ERB comment syntax. Should be <%#= without a space between. Leaving a space between ERB tags and the Ruby comment character can cause parser errors. In file: test_comment_syntax.html.erb:2 Bad ERB comment syntax. Should be <%# without a space between. Leaving a space between ERB tags and the Ruby comment character can cause parser errors. In file: test_comment_syntax.html.erb:6 2 error(s) were found in ERB files ``` --- lib/erb_lint/linters/comment_syntax.rb | 21 +++++--------------- spec/erb_lint/linters/comment_syntax_spec.rb | 20 ++++++++++++++++--- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/lib/erb_lint/linters/comment_syntax.rb b/lib/erb_lint/linters/comment_syntax.rb index 6a0aef9b..359e06b4 100644 --- a/lib/erb_lint/linters/comment_syntax.rb +++ b/lib/erb_lint/linters/comment_syntax.rb @@ -16,34 +16,23 @@ def run(processed_source) processed_source.ast.descendants(:erb).each do |erb_node| indicator_node, _, code_node, _ = *erb_node - next if indicator_node.nil? || code_node.nil? + next if code_node.nil? - indicator_node_str = indicator_node.deconstruct.last + indicator_node_str = indicator_node&.deconstruct&.last next if indicator_node_str == "#" - if indicator_node_str == "=" - range = find_range(erb_node, indicator_node_str) - source_range = processed_source.to_source_range(range) - - add_offense( - source_range, - <<~EOF.chomp - Bad ERB comment syntax. Should be `<%#=` without a space between. - Leaving a space between ERB tags and the Ruby comment character can cause parser errors. - EOF - ) - end - code_node_str = code_node.deconstruct.last next unless code_node_str.start_with?(" #") range = find_range(erb_node, code_node_str) source_range = processed_source.to_source_range(range) + correct_erb_tag = indicator_node_str == "=" ? "<%#=" : "<%#" + add_offense( source_range, <<~EOF.chomp - Bad ERB comment syntax. Should be `<%#` without a space between. + Bad ERB comment syntax. Should be #{correct_erb_tag} without a space between. Leaving a space between ERB tags and the Ruby comment character can cause parser errors. EOF ) diff --git a/spec/erb_lint/linters/comment_syntax_spec.rb b/spec/erb_lint/linters/comment_syntax_spec.rb index c098bd1e..4404a3bd 100644 --- a/spec/erb_lint/linters/comment_syntax_spec.rb +++ b/spec/erb_lint/linters/comment_syntax_spec.rb @@ -35,6 +35,20 @@ end context "when the ERB comment syntax is incorrect" do + let(:file) { <<~FILE } + <% # bad comment %> + FILE + + it "reports one offense" do + expect(subject.size).to(eq(1)) + end + + it "reports the suggested fix" do + expect(subject.first.message).to(include("Bad ERB comment syntax. Should be <%# without a space between.")) + end + end + + context "when the ERB comment syntax is incorrect multiple times in one file" do let(:file) { <<~FILE } <% # first bad comment %> <%= # second bad comment %> @@ -44,9 +58,9 @@ expect(subject.size).to(eq(2)) end - it "reports two offenses with their suggested fixes" do - expect(subject.first.message).to(include("Bad ERB comment syntax. Should be `<%#=` without a space between.")) - expect(subject.last.message).to(include("Bad ERB comment syntax. Should be `<%#` without a space between.")) + it "reports the suggested fixes" do + expect(subject.first.message).to(include("Bad ERB comment syntax. Should be <%# without a space between.")) + expect(subject.last.message).to(include("Bad ERB comment syntax. Should be <%#= without a space between.")) end end end From 7f0b494e498389eba0fc4181572ac24feb369ca9 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 21 Oct 2022 10:36:39 +0200 Subject: [PATCH 26/85] Bump nokogiri from 1.13.8 to 1.13.9 (#281) Bumps [nokogiri](https://github.com/sparklemotion/nokogiri) from 1.13.8 to 1.13.9. - [Release notes](https://github.com/sparklemotion/nokogiri/releases) - [Changelog](https://github.com/sparklemotion/nokogiri/blob/main/CHANGELOG.md) - [Commits](https://github.com/sparklemotion/nokogiri/compare/v1.13.8...v1.13.9) --- updated-dependencies: - dependency-name: nokogiri dependency-type: indirect ... Signed-off-by: dependabot[bot] Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 72c45979..f457acd5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -44,7 +44,7 @@ GEM nokogiri (>= 1.5.9) mini_portile2 (2.8.0) minitest (5.16.3) - nokogiri (1.13.8) + nokogiri (1.13.9) mini_portile2 (~> 2.8.0) racc (~> 1.4) parallel (1.22.1) From d0e72efca25db6b6467a2f83449227193a90ae65 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Wed, 26 Oct 2022 11:44:23 +0100 Subject: [PATCH 27/85] README: Add good and bad examples for `CommentSyntax` --- README.md | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 23ff64ad..de056cba 100644 --- a/README.md +++ b/README.md @@ -110,7 +110,7 @@ linters: | [ErbSafety](#ErbSafety) | No | detects unsafe interpolation of ruby data into various javascript contexts and enforce usage of safe helpers like `.to_json`. | | [Rubocop](#Rubocop) | No | runs RuboCop rules on ruby statements found in ERB templates | | [RequireScriptNonce](#RequireScriptNonce) | No | warns about missing [Content Security Policy nonces](https://guides.rubyonrails.org/security.html#content-security-policy) in script tags | -| CommentSyntax | No | detects bad comment syntax, ie `<% # comment %>` is not technically valid ERB but `<%# comment %>` is | +| [CommentSyntax](#CommentSyntax) | No | detects bad ERB comment syntax | ### DeprecatedClasses @@ -488,6 +488,27 @@ Linter-Specific Option | Description `allow_blank` | True or false, depending on whether or not the `type` attribute may be omitted entirely from a `