",
diff --git a/lib/theme_check/checks/missing_enable_comment.rb b/lib/theme_check/checks/missing_enable_comment.rb
index a559a42c..50588728 100644
--- a/lib/theme_check/checks/missing_enable_comment.rb
+++ b/lib/theme_check/checks/missing_enable_comment.rb
@@ -16,6 +16,10 @@ def on_comment(node)
@disabled_checks.update(node)
end
+ def on_inline_comment(node)
+ @disabled_checks.update(node)
+ end
+
def after_document(node)
checks_missing_end_index = @disabled_checks.checks_missing_end_index
return if checks_missing_end_index.empty?
diff --git a/lib/theme_check/disabled_checks.rb b/lib/theme_check/disabled_checks.rb
index 0dfd4a1c..0cabea26 100644
--- a/lib/theme_check/disabled_checks.rb
+++ b/lib/theme_check/disabled_checks.rb
@@ -67,7 +67,12 @@ def remove_disabled_offenses(checks)
private
def comment_text(node)
- node.value.nodelist.join
+ case node.type_name
+ when :comment
+ node.value.nodelist.join
+ when :inline_comment
+ node.markup.sub(/\s*#+\s*/, '')
+ end
end
def start_disabling?(text)
diff --git a/lib/theme_check/liquid_node.rb b/lib/theme_check/liquid_node.rb
index 40a45eee..d0dd4712 100644
--- a/lib/theme_check/liquid_node.rb
+++ b/lib/theme_check/liquid_node.rb
@@ -158,6 +158,11 @@ def comment?
@value.is_a?(Liquid::Comment)
end
+ # {% # comment %}
+ def inline_comment?
+ @value.is_a?(Liquid::InlineComment)
+ end
+
# Top level node of every liquid_file.
def document?
@value.is_a?(Liquid::Document)
diff --git a/lib/theme_check/liquid_visitor.rb b/lib/theme_check/liquid_visitor.rb
index e7fafa6d..e158e315 100644
--- a/lib/theme_check/liquid_visitor.rb
+++ b/lib/theme_check/liquid_visitor.rb
@@ -28,7 +28,7 @@ def visit(node)
call_checks(:after_node, node)
end
- @disabled_checks.update(node) if node.comment?
+ @disabled_checks.update(node) if node.comment? || node.inline_comment?
end
def call_checks(method, *args)
diff --git a/test/checks/missing_enable_comment_test.rb b/test/checks/missing_enable_comment_test.rb
index 8e4da08c..eecc36c3 100644
--- a/test/checks/missing_enable_comment_test.rb
+++ b/test/checks/missing_enable_comment_test.rb
@@ -3,91 +3,110 @@
require "minitest/focus"
class MissingEnableCommentTest < Minitest::Test
+ def comment_types
+ [
+ -> (text) { "{% comment %}#{text}{% endcomment %}" },
+ -> (text) { "{% # #{text} %}" },
+ ]
+ end
+
def test_always_enabled_by_default
refute(ThemeCheck::MissingEnableComment.new.can_disable?)
end
def test_no_default_noops
- offenses = analyze_theme(
- ThemeCheck::MissingEnableComment.new,
- "templates/index.liquid" => <<~END,
- {% comment %}theme-check-disable{% endcomment %}
- {% assign x = 1 %}
- {% comment %}theme-check-enable{% endcomment %}
- END
- )
- assert_offenses("", offenses)
+ comment_types.each do |comment|
+ offenses = analyze_theme(
+ ThemeCheck::MissingEnableComment.new,
+ "templates/index.liquid" => <<~END,
+ #{comment.call('theme-check-disable')}
+ {% assign x = 1 %}
+ #{comment.call('theme-check-enable')}
+ END
+ )
+ assert_offenses("", offenses)
+ end
end
def test_first_line_comment_disables_entire_file
- offenses = analyze_theme(
- ThemeCheck::MissingEnableComment.new,
- "templates/index.liquid" => <<~END,
- {% comment %}theme-check-disable{% endcomment %}
- {% assign x = 1 %}
- END
- )
- assert_offenses("", offenses)
+ comment_types.each do |comment|
+ offenses = analyze_theme(
+ ThemeCheck::MissingEnableComment.new,
+ "templates/index.liquid" => <<~END,
+ #{comment.call('theme-check-disable')}
+ {% assign x = 1 %}
+ END
+ )
+ assert_offenses("", offenses)
+ end
end
def test_non_first_line_comment_triggers_offense
- offenses = analyze_theme(
- ThemeCheck::MissingEnableComment.new,
- "templates/index.liquid" => <<~END,
- Hello, world
- {% comment %}theme-check-disable{% endcomment %} - {% assign x = 1 %} + comment_types.each do |comment| + offenses = analyze_theme( + ThemeCheck::MissingEnableComment.new, + "templates/index.liquid" => <<~END, +Hello, world
+ #{comment.call('theme-check-disable')} + {% assign x = 1 %} + END + ) + assert_offenses(<<~END, offenses) + All checks were disabled but not re-enabled with theme-check-enable at templates/index.liquid END - ) - assert_offenses(<<~END, offenses) - All checks were disabled but not re-enabled with theme-check-enable at templates/index.liquid - END + end end def test_specific_checks_disabled - offenses = analyze_theme( - ThemeCheck::MissingEnableComment.new, - Minitest::Test::TracerCheck.new, - "templates/index.liquid" => <<~END, -Hello, world
- {% comment %}theme-check-disable TracerCheck{% endcomment %} - {% assign x = 1 %} + comment_types.each do |comment| + offenses = analyze_theme( + ThemeCheck::MissingEnableComment.new, + Minitest::Test::TracerCheck.new, + "templates/index.liquid" => <<~END, +Hello, world
+ #{comment.call('theme-check-disable TracerCheck')} + {% assign x = 1 %} + END + ) + assert_offenses(<<~END, offenses) + TracerCheck was disabled but not re-enabled with theme-check-enable at templates/index.liquid END - ) - assert_offenses(<<~END, offenses) - TracerCheck was disabled but not re-enabled with theme-check-enable at templates/index.liquid - END + end end def test_specific_checks_disabled_and_reenabled - offenses = analyze_theme( - ThemeCheck::MissingEnableComment.new, - Minitest::Test::TracerCheck.new, - "templates/index.liquid" => <<~END, -Hello, world
- {% comment %}theme-check-disable TracerCheck, AnotherCheck{% endcomment %} - {% assign x = 1 %} + comment_types.each do |comment| + offenses = analyze_theme( + ThemeCheck::MissingEnableComment.new, + Minitest::Test::TracerCheck.new, + "templates/index.liquid" => <<~END, +Hello, world
+ #{comment.call('theme-check-disable TracerCheck, AnotherCheck')} + {% assign x = 1 %} + END + ) + assert_offenses(<<~END, offenses) + TracerCheck, AnotherCheck were disabled but not re-enabled with theme-check-enable at templates/index.liquid END - ) - assert_offenses(<<~END, offenses) - TracerCheck, AnotherCheck were disabled but not re-enabled with theme-check-enable at templates/index.liquid - END + end end def test_specific_checks_disabled_and_reenabled_with_all_checks_disabled - offenses = analyze_theme( - ThemeCheck::MissingEnableComment.new, - Minitest::Test::TracerCheck.new, - "templates/index.liquid" => <<~END, -Hello, world
- {% comment %}theme-check-disable TracerCheck, AnotherCheck{% endcomment %} - {% assign x = 1 %} - {% comment %}theme-check-disable{% endcomment %} - {% assign x = 1 %} + comment_types.each do |comment| + offenses = analyze_theme( + ThemeCheck::MissingEnableComment.new, + Minitest::Test::TracerCheck.new, + "templates/index.liquid" => <<~END, +Hello, world
+ #{comment.call('theme-check-disable TracerCheck, AnotherCheck')} + {% assign x = 1 %} + #{comment.call('theme-check-disable')} + {% assign x = 1 %} + END + ) + assert_offenses(<<~END, offenses) + All checks were disabled but not re-enabled with theme-check-enable at templates/index.liquid END - ) - assert_offenses(<<~END, offenses) - All checks were disabled but not re-enabled with theme-check-enable at templates/index.liquid - END + end end end diff --git a/test/disabled_checks_test.rb b/test/disabled_checks_test.rb index d3a675ad..515a986d 100644 --- a/test/disabled_checks_test.rb +++ b/test/disabled_checks_test.rb @@ -53,18 +53,35 @@ def setup @visitor = LiquidVisitor.new(@checks, @disabled_checks) end + def block_comment(text) + "{% comment %}#{text}{% endcomment %}" + end + + def inline_comment(text) + "{% # #{text} %}" + end + + def comment_types + [ + -> (text) { block_comment(text) }, + -> (text) { inline_comment(text) }, + ] + end + def test_ignore_all_checks - liquid_file = parse_liquid(<<~END) - {% comment %}theme-check-disable{% endcomment %} - {% assign x = 'x' %} - RegexError 1 - {% comment %}theme-check-enable{% endcomment %} - END - @visitor.visit_liquid_file(liquid_file) - @disabled_checks.remove_disabled_offenses(@checks) + comment_types.each do |comment| + liquid_file = parse_liquid(<<~END) + #{comment.call('theme-check-disable')} + {% assign x = 'x' %} + RegexError 1 + #{comment.call('theme-check-enable')} + END + @visitor.visit_liquid_file(liquid_file) + @disabled_checks.remove_disabled_offenses(@checks) - assert_empty(@assign_check.offenses) - assert_empty(@regex_check.offenses) + assert_empty(@assign_check.offenses) + assert_empty(@regex_check.offenses) + end end def test_ignore_all_checks_issue_583 @@ -86,137 +103,153 @@ def test_ignore_all_checks_issue_583 end def test_ignore_all_checks_without_end - liquid_file = parse_liquid(<<~END) - {% comment %}theme-check-disable{% endcomment %} - {% assign x = 'x' %} - RegexError 1 - END - @visitor.visit_liquid_file(liquid_file) - @disabled_checks.remove_disabled_offenses(@checks) + comment_types.each do |comment| + liquid_file = parse_liquid(<<~END) + #{comment.call('theme-check-disable')} + {% assign x = 'x' %} + RegexError 1 + END + @visitor.visit_liquid_file(liquid_file) + @disabled_checks.remove_disabled_offenses(@checks) - assert_empty(@assign_check.offenses) - assert_empty(@regex_check.offenses) + assert_empty(@assign_check.offenses) + assert_empty(@regex_check.offenses) + end end def test_ignore_all_checks_between_bounds - liquid_file = parse_liquid(<<~END) - {% assign x = 'x' %} - RegexError 1 - {% comment %}theme-check-disable{% endcomment %} - {% assign y = 'y' %} - RegexError 2 - {% comment %}theme-check-enable{% endcomment %} - {% assign z = 'z' %} - RegexError 3 - END - @visitor.visit_liquid_file(liquid_file) - @disabled_checks.remove_disabled_offenses(@checks) - - assert_includes(@assign_check.offenses.map(&:markup), "assign x = 'x' ") - refute_includes(@assign_check.offenses.map(&:markup), "assign y = 'y' ") - assert_includes(@assign_check.offenses.map(&:markup), "assign z = 'z' ") - assert_includes(@regex_check.offenses.map(&:markup), "RegexError 1") - refute_includes(@regex_check.offenses.map(&:markup), "RegexError 2") - assert_includes(@regex_check.offenses.map(&:markup), "RegexError 3") + comment_types.each do |comment| + liquid_file = parse_liquid(<<~END) + {% assign x = 'x' %} + RegexError 1 + #{comment.call('theme-check-disable')} + {% assign y = 'y' %} + RegexError 2 + #{comment.call('theme-check-enable')} + {% assign z = 'z' %} + RegexError 3 + END + @visitor.visit_liquid_file(liquid_file) + @disabled_checks.remove_disabled_offenses(@checks) + + assert_includes(@assign_check.offenses.map(&:markup), "assign x = 'x' ") + refute_includes(@assign_check.offenses.map(&:markup), "assign y = 'y' ") + assert_includes(@assign_check.offenses.map(&:markup), "assign z = 'z' ") + assert_includes(@regex_check.offenses.map(&:markup), "RegexError 1") + refute_includes(@regex_check.offenses.map(&:markup), "RegexError 2") + assert_includes(@regex_check.offenses.map(&:markup), "RegexError 3") + end end def test_ignore_specific_checks - liquid_file = parse_liquid(<<~END) - {% comment %}theme-check-disable AssignCheck{% endcomment %} - {% assign x = 'x' %} - RegexError 1 - {% comment %}theme-check-enable AssignCheck{% endcomment %} - END - @visitor.visit_liquid_file(liquid_file) - @disabled_checks.remove_disabled_offenses(@checks) + comment_types.each do |comment| + liquid_file = parse_liquid(<<~END) + #{comment.call('theme-check-disable AssignCheck')} + {% assign x = 'x' %} + RegexError 1 + #{comment.call('theme-check-enable AssignCheck')} + END + @visitor.visit_liquid_file(liquid_file) + @disabled_checks.remove_disabled_offenses(@checks) - assert_empty(@assign_check.offenses) - refute_empty(@regex_check.offenses) + assert_empty(@assign_check.offenses) + refute_empty(@regex_check.offenses) + end end def test_ignore_multiple_checks - liquid_file = parse_liquid(<<~END) - {% comment %}theme-check-disable AssignCheck, RegexCheck{% endcomment %} - {% assign x = 'x' %} - RegexError 1 - {% comment %}theme-check-enable AssignCheck, RegexCheck{% endcomment %} - END - @visitor.visit_liquid_file(liquid_file) - @disabled_checks.remove_disabled_offenses(@checks) + comment_types.each do |comment| + liquid_file = parse_liquid(<<~END) + #{comment.call('theme-check-disable AssignCheck, RegexCheck')} + {% assign x = 'x' %} + RegexError 1 + #{comment.call('theme-check-enable AssignCheck, RegexCheck')} + END + @visitor.visit_liquid_file(liquid_file) + @disabled_checks.remove_disabled_offenses(@checks) - assert_empty(@assign_check.offenses) - assert_empty(@regex_check.offenses) + assert_empty(@assign_check.offenses) + assert_empty(@regex_check.offenses) + end end def test_enable_specific_checks_individually - liquid_file = parse_liquid(<<~END) - {% comment %}theme-check-disable AssignCheck, RegexCheck{% endcomment %} - {% assign x = 'x' %} - RegexError 1 - {% comment %}theme-check-enable AssignCheck{% endcomment %} - {% assign y = 'y' %} - RegexError 2 - {% comment %}theme-check-enable RegexCheck{% endcomment %} - {% assign z = 'z' %} - RegexError 3 - END - @visitor.visit_liquid_file(liquid_file) - @disabled_checks.remove_disabled_offenses(@checks) - - refute_empty(@assign_check.offenses) - refute_includes(@assign_check.offenses.map(&:markup), "assign x = 'x' ") - assert_includes(@assign_check.offenses.map(&:markup), "assign y = 'y' ") - assert_includes(@assign_check.offenses.map(&:markup), "assign z = 'z' ") - - refute_empty(@regex_check.offenses) - refute_includes(@regex_check.offenses.map(&:markup), "RegexError 1") - refute_includes(@regex_check.offenses.map(&:markup), "RegexError 2") - assert_includes(@regex_check.offenses.map(&:markup), "RegexError 3") + comment_types.each do |comment| + liquid_file = parse_liquid(<<~END) + #{comment.call('theme-check-disable AssignCheck, RegexCheck')} + {% assign x = 'x' %} + RegexError 1 + #{comment.call('theme-check-enable AssignCheck')} + {% assign y = 'y' %} + RegexError 2 + #{comment.call('theme-check-enable RegexCheck')} + {% assign z = 'z' %} + RegexError 3 + END + @visitor.visit_liquid_file(liquid_file) + @disabled_checks.remove_disabled_offenses(@checks) + + refute_empty(@assign_check.offenses) + refute_includes(@assign_check.offenses.map(&:markup), "assign x = 'x' ") + assert_includes(@assign_check.offenses.map(&:markup), "assign y = 'y' ") + assert_includes(@assign_check.offenses.map(&:markup), "assign z = 'z' ") + + refute_empty(@regex_check.offenses) + refute_includes(@regex_check.offenses.map(&:markup), "RegexError 1") + refute_includes(@regex_check.offenses.map(&:markup), "RegexError 2") + assert_includes(@regex_check.offenses.map(&:markup), "RegexError 3") + end end def test_comments_can_have_spaces - liquid_file = parse_liquid(<<~END) - {% comment %} theme-check-disable {% endcomment %} - {% assign x = 'x' %} - RegexError 1 - {% comment %} theme-check-enable {% endcomment %} - END - @visitor.visit_liquid_file(liquid_file) - @disabled_checks.remove_disabled_offenses(@checks) + comment_types.each do |comment| + liquid_file = parse_liquid(<<~END) + #{comment.call(' theme-check-disable ')} + {% assign x = 'x' %} + RegexError 1 + #{comment.call(' theme-check-enable ')} + END + @visitor.visit_liquid_file(liquid_file) + @disabled_checks.remove_disabled_offenses(@checks) - assert_empty(@assign_check.offenses) - assert_empty(@regex_check.offenses) + assert_empty(@assign_check.offenses) + assert_empty(@regex_check.offenses) + end end def test_ignore_disable_check_that_cant_be_disabled - RegexCheck.can_disable(false) - liquid_file = parse_liquid(<<~END) - {% comment %} theme-check-disable {% endcomment %} - RegexError 1 - {% comment %} theme-check-enable {% endcomment %} - {% comment %} theme-check-disable RegexCheck {% endcomment %} - RegexError 2 - {% comment %} theme-check-enable RegexCheck {% endcomment %} - END - @visitor.visit_liquid_file(liquid_file) - @disabled_checks.remove_disabled_offenses(@checks) - RegexCheck.can_disable(true) - - assert_empty(@assign_check.offenses) - assert_includes(@regex_check.offenses.map(&:markup), "RegexError 1") - assert_includes(@regex_check.offenses.map(&:markup), "RegexError 2") + comment_types.each do |comment| + RegexCheck.can_disable(false) + liquid_file = parse_liquid(<<~END) + #{comment.call(' theme-check-disable ')} + RegexError 1 + #{comment.call(' theme-check-enable ')} + #{comment.call(' theme-check-disable RegexCheck ')} + RegexError 2 + #{comment.call(' theme-check-enable RegexCheck ')} + END + @visitor.visit_liquid_file(liquid_file) + @disabled_checks.remove_disabled_offenses(@checks) + RegexCheck.can_disable(true) + + assert_empty(@assign_check.offenses) + assert_includes(@regex_check.offenses.map(&:markup), "RegexError 1") + assert_includes(@regex_check.offenses.map(&:markup), "RegexError 2") + end end def test_can_disable_check_that_run_on_end - liquid_file = parse_liquid(<<~END) - {% comment %}theme-check-disable OnEndCheck{% endcomment %} - Hello there - END - @visitor.visit_liquid_file(liquid_file) - @checks.call(:on_end) - @disabled_checks.remove_disabled_offenses(@checks) - - assert_empty(@on_end_check.offenses.map(&:theme_file)) + comment_types.each do |comment| + liquid_file = parse_liquid(<<~END) + #{comment.call('theme-check-disable OnEndCheck')} + Hello there + END + @visitor.visit_liquid_file(liquid_file) + @checks.call(:on_end) + @disabled_checks.remove_disabled_offenses(@checks) + + assert_empty(@on_end_check.offenses.map(&:theme_file)) + end end def test_can_ignore_check_using_pattern diff --git a/test/liquid_node_test.rb b/test/liquid_node_test.rb index c8c1ff03..d44f160c 100644 --- a/test/liquid_node_test.rb +++ b/test/liquid_node_test.rb @@ -26,6 +26,10 @@ def test_markup {{ 'x' }} + {% + # inline comment + # block + %} END assert_can_find_node_with_markup(root, "if true ") @@ -39,6 +43,7 @@ def test_markup assert_can_find_node_with_markup(root, "if\n true") assert_can_find_node_with_markup(root, " 'x' ") assert_can_find_node_with_markup(root, "\n 'x'\n ") + assert_can_find_node_with_markup(root, "# inline comment\n # block\n ") end def test_inside_liquid_tag? @@ -547,6 +552,16 @@ def test_outer_markup_repeated_comments COMMENT end + def test_outer_markup_repeated_comments_inline_comment + liquid = <<~LIQUID + {% # theme-check-disable foo %} + {% # theme-check-disable bar %} + {%# theme-check-disable baz%} + LIQUID + assert_can_find_node_with_outer_markup('{% # theme-check-disable foo %}', liquid) + assert_can_find_node_with_outer_markup('{%# theme-check-disable baz%}', liquid) + end + def test_inner_markup assert_node_inner_markup_equals('', '{{x}}') assert_node_inner_markup_equals('', '{{ x }}') diff --git a/theme-check.gemspec b/theme-check.gemspec index 315a6c29..fa50b331 100644 --- a/theme-check.gemspec +++ b/theme-check.gemspec @@ -24,7 +24,7 @@ Gem::Specification.new do |spec| spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } spec.require_paths = ["lib"] - spec.add_dependency('liquid', '>= 5.2.0') + spec.add_dependency('liquid', '>= 5.4.0') spec.add_dependency('nokogiri', '>= 1.12') spec.add_dependency('parser', '~> 3') end