Add support for new comment syntax (#533)

Fixes #179
Shopify · Jul 29, 2022 · e748a71 · e748a71
1 parent 73245f6
commit e748a71
Show file tree

Hide file tree

Showing 14 changed files with 287 additions and 207 deletions.
diff --git a/README.md b/README.md
@@ -115,32 +115,31 @@ See [config/default.yml](config/default.yml) for available options & defaults.
 Use Liquid comments to disable and re-enable all checks for a section of your template:
 
 ```liquid
-{% comment %}theme-check-disable{% endcomment %}
+{% # theme-check-disable %}
 {% assign x = 1 %}
-{% comment %}theme-check-enable{% endcomment %}
+{% # theme-check-enable %}
 ```
 
 Disable a specific check by including it in the comment:
 
 ```liquid
-{% comment %}theme-check-disable UnusedAssign{% endcomment %}
+{% # theme-check-disable UnusedAssign %}
 {% assign x = 1 %}
-{% comment %}theme-check-enable UnusedAssign{% endcomment %}
+{% # theme-check-enable UnusedAssign %}
 ```
 
 Disable multiple checks by including them as a comma-separated list:
 
 ```liquid
-{% comment %}theme-check-disable UnusedAssign,SpaceInsideBraces{% endcomment %}
+{% # theme-check-disable UnusedAssign,SpaceInsideBraces %}
 {%assign x = 1%}
-{% comment %}theme-check-enable UnusedAssign,SpaceInsideBraces{% endcomment %}
+{% # theme-check-enable UnusedAssign,SpaceInsideBraces %}
 ```
 
 Disable checks for the _entire document_ by placing the comment on the first line:
 
 ```liquid
-{% comment %}theme-check-disable SpaceInsideBraces{% endcomment %}
-
+{% # theme-check-disable SpaceInsideBraces %}
 {%assign x = 1%}
 ```
 

diff --git a/docs/checks/asset_size_javascript.md b/docs/checks/asset_size_javascript.md
@@ -57,9 +57,9 @@ This includes theme and remote scripts.
 When you can't do anything about it, it is preferable to disable this rule using the comment syntax:
 
 ```
-{% comment %}theme-check-disable AssetSizeJavaScript{% endcomment %}
+{% # theme-check-disable AssetSizeJavaScript %}
 <script src="https://code.jquery.com/jquery-3.6.0.min.js" defer></script>
-{% comment %}theme-check-enable AssetSizeJavaScript{% endcomment %}
+{% # theme-check-enable AssetSizeJavaScript %}
 ```
 
 This makes disabling the rule an explicit affair and shows that the code is smelly.

diff --git a/docs/checks/missing_enable_comment.md b/docs/checks/missing_enable_comment.md
@@ -12,7 +12,7 @@ This check aims at eliminating missing `theme-check-enable` comments.
 <!doctype html>
 <html>
   <head>
-    {% comment %}theme-check-disable ParserBlockingJavaScript{% endcomment %}
+    {% # theme-check-disable ParserBlockingJavaScript %}
     <script src="https://cdnjs.com/jquery.min.js"></script>
   </head>
   <body>
@@ -27,9 +27,9 @@ This check aims at eliminating missing `theme-check-enable` comments.
 <!doctype html>
 <html>
   <head>
-    {% comment %}theme-check-disable ParserBlockingJavaScript{% endcomment %}
+    {% # theme-check-disable ParserBlockingJavaScript %}
     <script src="https://cdnjs.com/jquery.min.js"></script>
-    {% comment %}theme-check-enable ParserBlockingJavaScript{% endcomment %}
+    {% # theme-check-enable ParserBlockingJavaScript %}
   </head>
   <body>
     <!-- ... -->

diff --git a/docs/checks/nested_snippet.md b/docs/checks/nested_snippet.md
@@ -9,32 +9,32 @@ This check is aimed at eliminating excessive nesting of snippets.
 :-1: Examples of **incorrect** code for this check:
 
 ```liquid
-{% comment %}templates/index.liquid{% endcomment %}
+{% # templates/index.liquid %}
   {% render 'one' %}
 
-{% comment %}snippets/one.liquid{% endcomment %}
+{% # snippets/one.liquid %}
   {% render 'two' %}
 
-{% comment %}snippets/two.liquid{% endcomment %}
+{% # snippets/two.liquid %}
   {% render 'three' %}
 
-{% comment %}snippets/three.liquid{% endcomment %}
+{% # snippets/three.liquid %}
   {% render 'four' %}
 
-{% comment %}snippets/four.liquid{% endcomment %}
+{% # snippets/four.liquid %}
   ok
 ```
 
 :+1: Examples of **correct** code for this check:
 
 ```liquid
-{% comment %}templates/index.liquid{% endcomment %}
+{% # templates/index.liquid %}
   {% render 'one' %}
 
-{% comment %}snippets/one.liquid{% endcomment %}
+{% # snippets/one.liquid %}
   {% render 'two' %}
 
-{% comment %}snippets/two.liquid{% endcomment %}
+{% # snippets/two.liquid %}
   ok
 ```
 

diff --git a/docs/checks/translation_key_exists.md b/docs/checks/translation_key_exists.md
@@ -9,30 +9,30 @@ This check is aimed at eliminating the use of translations that do not exist.
 :-1: Examples of **incorrect** code for this check:
 
 ```liquid
-{% comment %}locales/en.default.json{% endcomment %}
+{% # locales/en.default.json %}
 {
   "greetings": "Hello, world!",
   "general": {
     "close": "Close"
   }
 }
 
-{% comment %}templates/index.liquid{% endcomment %}
+{% # templates/index.liquid %}
 {{ "notfound" | t }}
 ```
 
 :+1: Examples of **correct** code for this check:
 
 ```liquid
-{% comment %}locales/en.default.json{% endcomment %}
+{% # locales/en.default.json %}
 {
   "greetings": "Hello, world!",
   "general": {
     "close": "Close"
   }
 }
 
-{% comment %}templates/index.liquid{% endcomment %}
+{% # templates/index.liquid %}
 {{ "greetings" | t }}
 {{ "general.close" | t }}
 ```

diff --git a/docs/checks/valid_html_translation.md b/docs/checks/valid_html_translation.md
@@ -18,7 +18,7 @@ This check is aimed at eliminating invalid HTML in translations.
 :+1: Examples of **correct** code for this check:
 
 ```liquid
-{% comment %}locales/en.default.json{% endcomment %}
+{% # locales/en.default.json %}
 {
   "hello_html": "<h1>Hello, world</h1>",
   "image_html": "<img src='spongebob.png'>",

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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,
-        <p>Hello, world</p>
-        {% comment %}theme-check-disable{% endcomment %}
-        {% assign x = 1 %}
+    comment_types.each do |comment|
+      offenses = analyze_theme(
+        ThemeCheck::MissingEnableComment.new,
+        "templates/index.liquid" => <<~END,
+          <p>Hello, world</p>
+          #{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,
-        <p>Hello, world</p>
-        {% 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,
+          <p>Hello, world</p>
+          #{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,
-        <p>Hello, world</p>
-        {% 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,
+          <p>Hello, world</p>
+          #{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,
-        <p>Hello, world</p>
-        {% 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,
+          <p>Hello, world</p>
+          #{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