Merge pull request #57 from github/kh-rename-from-counter

khiga8 · web-flow · commit dee42cf048de · 2023-03-31T11:21:01.000-04:00
Start migrating rules away from counter system
diff --git a/README.md b/README.md
@@ -55,7 +55,7 @@ linters:
 
 ## Rules
 
-- [GitHub::Accessibility::AvoidBothDisabledAndAriaDisabledCounter](./docs/rules/accessibility/avoid-both-disabled-and-aria-disabled-counter.md)
+- [GitHub::Accessibility::AvoidBothDisabledAndAriaDisabledCounter](./docs/rules/accessibility/avoid-both-disabled-and-aria-disabled.md)
 - [GitHub::Accessibility::AvoidGenericLinkTextCounter](./docs/rules/accessibility/avoid-generic-link-text-counter.md)
 - [GitHub::Accessibility::DisabledAttributeCounter](./docs/rules/accessibility/disabled-attribute-counter.md)
 - [GitHub::Accessibility::LandmarkHasLabelCounter](./docs/rules/accessibility/landmark-has-label-counter.md)
diff --git a/docs/counter-migration-guide.md b/docs/counter-migration-guide.md
@@ -0,0 +1,62 @@
+# Counter migration guide
+
+[ERBLint v0.4.0](https://github.com/Shopify/erb-lint/releases/tag/v0.4.0) introduces support for inline lint rule disables.
+
+Since an inline disable feature is now natively available, it is time to move away from the in-house (hacky) counter system we've used internally over the years and in this library. 🎉
+
+Our latest `erblint-github` release removes the `-Counter` prefix from all of the lint rules. Please update your `.erb-lint.yml` accordingly.
+
+If your configuration looks something like:
+
+```yaml
+---
+linters:
+  GitHub::Accessibility::AvoidBothDisabledAndAriaCounter:
+    enabled: true
+  GitHub::Accessibility::AvoidGenericLinkTextCounter:
+    enabled: true
+  GitHub::Accessibility::DisabledAttributeCounter:
+    enabled: true
+```
+
+It should become the following, with `-Counter` removed to make sure the rules run correctly.
+
+```yaml
+---
+linters:
+  GitHub::Accessibility::AvoidBothDisabledAndAria:
+    enabled: true
+  GitHub::Accessibility::AvoidGenericLinkText:
+    enabled: true
+  GitHub::Accessibility::DisabledAttribute:
+    enabled: true
+```
+
+## Easing migration
+
+In order to ease migration for codebases where counter comments are in place (especially large codebases), we will continue to support counters for existing lint rules for a few releases until we deprecate it completely. This should allow you to migrate rules one-by-one, rather than all at once.
+
+With this release, the counter system will now be toggled off by default, so please explicitly enable them in your `.erb-lint.yml` config if you would like to enable counters.
+
+```yaml
+---
+linters:
+  GitHub::Accessibility::AvoidBothDisabledAndAria:
+    enabled: true
+    counter_enabled: true
+  GitHub::Accessibility::AvoidGenericLinkText:
+    enabled: true
+    counter_enabled: true
+```
+
+With this `counter_enabled: true` config, your counter comments like `<%# erblint:counter GitHub::Accessibility::AvoidBothDisabledAndAriaCounter 1` should work as it did before.
+
+However, we will drop support for the counter system within the next few releases so please take time to migrate your counter comments to native inline disable comments. Any new rules added to this library will not support the counter system.
+
+Once your files do not have any counter comments, remove the `counter_enabled: true` from your configuration to ensure no new ones are added.
+
+## Automate migration
+
+Adding inline disables for large codebases can be extremely tedious. We recommend using a script to automate this process.
+
+<!-- TODO: Add script that consumers can use -->
diff --git a/docs/rules/accessibility/avoid-both-disabled-and-aria-disabled.md b/docs/rules/accessibility/avoid-both-disabled-and-aria-disabled.md
diff --git a/lib/erblint-github/linters/github/accessibility/avoid_both_disabled_and_aria_disabled.rb b/lib/erblint-github/linters/github/accessibility/avoid_both_disabled_and_aria_disabled.rb
@@ -6,13 +6,18 @@ module ERBLint
   module Linters
     module GitHub
       module Accessibility
-        class AvoidBothDisabledAndAriaDisabledCounter < Linter
+        class AvoidBothDisabledAndAriaDisabled < Linter
           include ERBLint::Linters::CustomHelpers
           include LinterRegistry
 
           ELEMENTS_WITH_NATIVE_DISABLED_ATTRIBUTE_SUPPORT = %w[button fieldset input optgroup option select textarea].freeze
           MESSAGE = "[aria-disabled] may be used in place of native HTML [disabled] to allow tab-focus on an otherwise ignored element. Setting both attributes is contradictory."
 
+          class ConfigSchema < LinterConfig
+            property :counter_enabled, accepts: [true, false], default: false, reader: :counter_enabled?
+          end
+          self.config_schema = ConfigSchema
+
           def run(processed_source)
             tags(processed_source).each do |tag|
               next if tag.closing?
@@ -22,20 +27,8 @@ def run(processed_source)
               generate_offense(self.class, processed_source, tag)
             end
 
-            counter_correct?(processed_source)
-          end
-
-          def autocorrect(processed_source, offense)
-            return unless offense.context
-
-            lambda do |corrector|
-              if processed_source.file_content.include?("erblint:counter #{simple_class_name}")
-                # update the counter if exists
-                corrector.replace(offense.source_range, offense.context)
-              else
-                # add comment with counter if none
-                corrector.insert_before(processed_source.source_buffer.source_range, "#{offense.context}\n")
-              end
+            if @config.counter_enabled?
+              counter_correct?(processed_source)
             end
           end
         end
diff --git a/test/linters/accessibility/avoid_both_disabled_and_aria_disabled_test.rb b/test/linters/accessibility/avoid_both_disabled_and_aria_disabled_test.rb
@@ -4,7 +4,7 @@
 
 class AvoidBothDisabledAndAriaDisabled < LinterTestCase
   def linter_class
-    ERBLint::Linters::GitHub::Accessibility::AvoidBothDisabledAndAriaDisabledCounter
+    ERBLint::Linters::GitHub::Accessibility::AvoidBothDisabledAndAriaDisabled
   end
 
   ELEMENTS_WITH_NATIVE_DISABLED_ATTRIBUTE_SUPPORT = %w[button fieldset input optgroup option select textarea].freeze
@@ -15,7 +15,7 @@ def test_warns_if_both_disabled_and_aria_disabled_set_on_html_elements_with_disa
     end.join
     @linter.run(processed_source)
 
-    assert_equal @linter.offenses.count, 8
+    assert_equal @linter.offenses.count, 7
   end
 
   def test_does_not_warn_if_only_disabled_attribute_is_set
@@ -36,35 +36,24 @@ def test_does_not_warn_if_only_aria_disabled_attribute_is_set
     assert_empty @linter.offenses
   end
 
-  def test_does_not_raise_when_ignore_comment_with_correct_count
-    @file = <<~ERB
-      <%# erblint:counter GitHub::Accessibility::AvoidBothDisabledAndAriaDisabledCounter 1 %>
-      <button disabled aria-disabled="true">Some text</span>
-    ERB
-
+  def test_adds_extra_offense_to_add_counter_comment_if_counter_config_enabled
+    @file = ELEMENTS_WITH_NATIVE_DISABLED_ATTRIBUTE_SUPPORT.map do |element|
+      "<#{element} aria-disabled='true' disabled> </#{element}>"
+    end.join
+    @linter.config.counter_enabled = true
     @linter.run(processed_source)
-    assert_empty @linter.offenses
-  end
 
-  def test_does_not_autocorrect_when_ignores_are_correct
-    @file = <<~ERB
-      <%# erblint:counter GitHub::Accessibility::AvoidBothDisabledAndAriaDisabledCounter 1 %>
-      <button disabled aria-disabled="true">Some text</button>
-    ERB
-
-    assert_equal @file, corrected_content
+    assert_equal @linter.offenses.count, 8
+    assert_match(/If you must, add <%# erblint:counter GitHub::Accessibility::AvoidBothDisabledAndAriaDisabled 7 %> to bypass this check/, @linter.offenses.last.message)
   end
 
-  def test_does_autocorrect_when_ignores_are_not_correct
+  def test_does_not_raise_when_ignore_comment_with_correct_count_if_counter_enabled
     @file = <<~ERB
-      <button disabled aria-disabled="true">Some text</button>
-    ERB
-    refute_equal @file, corrected_content
-
-    expected_content = <<~ERB
       <%# erblint:counter GitHub::Accessibility::AvoidBothDisabledAndAriaDisabledCounter 1 %>
       <button disabled aria-disabled="true">Some text</button>
     ERB
-    assert_equal expected_content, corrected_content
+    @linter.config.counter_enabled = true
+    @linter.run(processed_source)
+    assert_empty @linter.offenses
   end
 end
