[Feature] Allow rule to be disabled by comment by khiga8 · Pull Request #249 · Shopify/erb_lint

khiga8 · 2022-02-04T19:41:06Z

Fixes: #243

Context

For codebases with a lot of violations of a certain rule, it would be helpful to allow rules to be disabled on a per violation basis so that the violations can be progressively addressed while preventing new violations from being introduced.

For example, rubocop allows rules to be disabled with comments:

# rubocop:disable SomeRuleName

Proposal

We allow offenses to be ignored when there is a disable comment appearing as part of the lines spanning the reported offense.

examples

Offenses that include a disable comment on offending lines will be ignored.

<hr /> <%# erblint:disable-line SelfClosingTag %>

We also allow multiple rule disables:

<span>some text</span> <%# erblint:disable-line FakeLinter1, FakeLinter2 %>

This also introduces NoUnusedDisable lint rule which can be configured to detect any unused disable comments.

Approach

This PR aims for backward compatibility. Consequently, we only update linter.rb and runner.rb so no changes should be required for existing custom rules that consumers may have created following how rules have been defined in this repo.

We introduce run_and_set_offense_status method which will be called by the ERBLint::Runner instead of run. This method will call linter.run and then set status of offense based on comment criteria described above.

When NoUnusedDisable is enabled, it runs in runner.rb after all other linters have ran. Then it will go through the file and report any unused disable comments.

What should reviewers focus on?

I went with what seems like a backward-compatible approach though it may not be the most efficient. Open to thoughts. I'd appreciate any feedback. Is the logic for how we decide to ignore an offense sufficient?

To do

Add tests for various scenarios
Report unused disable comments (ideally backwards compatible)
Get feedback on approach

lib/erb_lint/linters/no_unused_disable.rb

koddsson

This is really cool!

@rafaelfranca; what do you think about this change? I know @khiga8 wants to clean it up a bit more if this is something we want to merge so comments and feedback on an approach would be really cool.

rafaelfranca · 2022-02-25T21:47:09Z

Feature makes sense to me. Do you want to bring this to the finish line?

khiga8 · 2022-02-25T23:02:06Z

@rafaelfranca

Sure, I'd love to bring it to finish line. I will probably just clean up the messy code in no_unused_disable but otherwise I've written tests and it's basically ready 👍.

Does the current approach seem ok? I prioritized making non-breaking changes, so there are some inefficiencies.

I also want to get your thoughts on the disable comment placement and format. Currently it's set up so the disable comment can be placed on preceding line or any part of offending line:

1. Comment on offending lines

<hr /> <%# erblint:disable SelfClosingTag %>

2. Comment on previous line

<%# erblint:disable SelfClosingTag %>
<hr />

I just had a look at rubocop disable comment guide, and eslint disable comment guide, both of which take different disable comment approaches.

Do we want to align with either of those? Can you think of any weirdness that could emerge from this format approach, especially when considering we might want to support something like file-level disables?

khiga8 · 2022-03-08T03:40:11Z

@rafaelfranca This is ready for another look 🙇‍♀️

rafaelfranca · 2022-03-16T18:43:44Z

I think we should align with rubocop in this case.

lib/erb_lint/runner.rb

lib/erb_lint/linter.rb

README.md

khiga8 · 2022-03-27T20:44:39Z

I decided to remove support for comment on previous lines, so we only have support for disable comment on any part of the offending lines. This is somewhat similar to Rubocop:

One or more cops can be disabled on a single line with an end-of-line comment.

However, with, we may have offenses that span more than one line, so we allow comment to be on any part of the offending lines. Rubocop has support for formats like # rubocop:disable, # rubocop:enable but that seems complex so I decided not to pursue for this PR

This is ready for another review! cc: @rafaelfranca

This commit introduces a `run_and_clear_ignored_offenses` method which filters out offenses which may include a comment in the format ``` <%# erblint:disable #{offense.linter.simple_class_name} %> ``` as either part of the offending lines, or as a comment in the previous line.

This commit adds support for the no_unused_disable linter. This can be configured in `erb-lint.yml`. If there are any unnecessary disable statements, these will be reported. This is a special linter that must run after all other linters.

khiga8 · 2023-03-07T23:44:04Z

spec/erb_lint/cli_spec.rb

      end
    end

+    context "with --disable-inline-configs" do


I introduced a new config.

khiga8 · 2023-03-07T23:48:30Z

Made the following updates since the last time this was formally reviewed.

erblint:disable --> erblint:disable-line (technically an offense can span many lines, and this comment can be placed on any part of the offending line. would be open to other suggestions. this seems more in line with other linters)
added support for a CLI config that allows ignoring inline comments, --disable-inline-configs, and added test coverage
allow erblint:disable-line comment to be a Ruby-style comment, and added test coverage
moved some utils to lib/erb_lint/utils/inline_configs.rb and added test coverage

I would like there to be a command we can run to automate adding inline disables in the relevant places but I think that should be done in a separate PR.

This is ready for another review!

jonrohan

Fantastic 👍🏻

rafaelfranca · 2023-03-17T19:39:11Z

spec/spec_helper.rb


 require "erb_lint/all"
+
+def source_range_for_code(processed_source, code)


This is adding this method to all objects. Let's add it to a module and call it from the module instead

Addressed in: f52fc0f.

rafaelfranca · 2023-03-17T19:42:06Z

The only thing that I don't think I understood is why we are using erblint:disable-line instead of only erblint:disable if the comments can span multiple lines.

jonrohan · 2023-03-17T19:48:51Z

The only thing that I don't think I understood is why we are using erblint:disable-line instead of only erblint:disable if the comments can span multiple lines.

I think the distinction helps when turning off a rule for the whole file vs, just on a line. Having the extra functionality of file/line would let us also support this case:

I was thinking in a follow up, we could add erblint:disable to disable the whole file. In the meantime, what we need is a line disable.

khiga8 · 2023-03-20T17:52:00Z

The only thing that I don't think I understood is why we are using

What @jonrohan said! If you have a different suggestion or would like to talk this through, let me know!

rafaelfranca · 2023-03-24T19:28:34Z

erblint is modelled after rubocop. Rubocop uses the same disable directive for line and block. Did you find any reason why we need two different directives for line and block disabling?

jonrohan · 2023-03-24T19:46:14Z

erblint is modelled after rubocop. Rubocop uses the same disable directive for line and block. Did you find any reason why we need two different directives for line and block disabling?

Ah, ok. Deep down I felt like the eslint system was more flexible/functional, but didn't have an understanding on rubocop's stance on this. I found this PR/thread that the rubocop contributors responded to and now I feel like it's best to stick with what they do. 👍🏻

Sorry for the code review flip flopping here @khiga8

khiga8 · 2023-03-24T19:51:28Z

erblint is modelled after rubocop
now I feel like it's best to stick with what they do. 👍🏻

I'm fine aligning with Rubocop too. Swapped it back to erblint:disable in 0afcd51.

This PR makes possible to disable a rule in a erb line. The implementation follows the [Standard way](https://github.com/standardrb/standard?tab=readme-ov-file#ignoring-errors) to disable rules, in the same way that [ERBLint allows to disable rules inline](https://github.com/Shopify/erb_lint?tab=readme-ov-file#disable-rule-at-offense-level). The implementation is very similar this [pull request](Shopify/erb_lint#249). The strategy consist of 1. Use a regex to see which line skipped rules with the `<%# herb:disable some-rule %>` syntax 2. Partition the offenses collected by the visitor based on the rules that where disabled for each line. 3. Report in the reporters the number of offenses. Here is a screenshot of how the implementation looks in the Linter CLI. ```erb <div> <h1 class="<%= classes %>"> <%= title %> </h1> <DIV>hello</DIV> <DIV>hello</DIV> <%# herb:disable html-tag-name-lowercase %> <% %> <%# herb:disable erb-no-empty-tags %> </div> ``` <img width="790" height="620" alt="image" src="https://github.com/user-attachments/assets/e38c35bd-f96c-455a-b4a1-5818c05c5c3a" /> Future work: - Be able to skip the skipped lines (useful when we want to find where are the rules skipped). - Allow the LSP to make a code action with an autocorrect that adds the line skipping. - Allow disable multiple rules in one line (this should be pretty straightforward in a follow up PR). - Add another linter rule to avoid putting disable comments that not disable anything (like erb_lint NoUnusedDisable). Resolves #270

…th#531) This PR makes possible to disable a rule in a erb line. The implementation follows the [Standard way](https://github.com/standardrb/standard?tab=readme-ov-file#ignoring-errors) to disable rules, in the same way that [ERBLint allows to disable rules inline](https://github.com/Shopify/erb_lint?tab=readme-ov-file#disable-rule-at-offense-level). The implementation is very similar this [pull request](Shopify/erb_lint#249). The strategy consist of 1. Use a regex to see which line skipped rules with the `<%# herb:disable some-rule %>` syntax 2. Partition the offenses collected by the visitor based on the rules that where disabled for each line. 3. Report in the reporters the number of offenses. Here is a screenshot of how the implementation looks in the Linter CLI. ```erb <div> <h1 class="<%= classes %>"> <%= title %> </h1> <DIV>hello</DIV> <DIV>hello</DIV> <%# herb:disable html-tag-name-lowercase %> <% %> <%# herb:disable erb-no-empty-tags %> </div> ``` <img width="790" height="620" alt="image" src="https://github.com/user-attachments/assets/e38c35bd-f96c-455a-b4a1-5818c05c5c3a" /> Future work: - Be able to skip the skipped lines (useful when we want to find where are the rules skipped). - Allow the LSP to make a code action with an autocorrect that adds the line skipping. - Allow disable multiple rules in one line (this should be pretty straightforward in a follow up PR). - Add another linter rule to avoid putting disable comments that not disable anything (like erb_lint NoUnusedDisable). Resolves marcoroth#270

khiga8 force-pushed the kh-allow_linter_disable_by_comment branch 4 times, most recently from f9b67b2 to 0bb0edd Compare February 5, 2022 01:48

This comment was marked as outdated.

Sign in to view

khiga8 commented Feb 5, 2022

View reviewed changes

lib/erb_lint/linters/no_unused_disable.rb Outdated Show resolved Hide resolved

khiga8 force-pushed the kh-allow_linter_disable_by_comment branch from 0bb0edd to b7c8b67 Compare February 5, 2022 06:35

khiga8 marked this pull request as ready for review February 7, 2022 15:42

koddsson reviewed Feb 9, 2022

View reviewed changes

khiga8 changed the title ~~[Feature][WIP] Allow rule to be disabled by comment~~ [Feature][Needs feedback] Allow rule to be disabled by comment Feb 15, 2022

smockle mentioned this pull request Feb 22, 2022

Port over counter implementation and introduce linter against title attribute github/erblint-github#19

Merged

khiga8 changed the title ~~[Feature][Needs feedback] Allow rule to be disabled by comment~~ [Feature] Allow rule to be disabled by comment Mar 3, 2022

rafaelfranca reviewed Mar 16, 2022

View reviewed changes

khiga8 commented Mar 17, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

github-actions bot added cla-needed and removed cla-needed labels Mar 2, 2023

khiga8 force-pushed the kh-allow_linter_disable_by_comment branch from 9e4f8b0 to 85fe954 Compare March 3, 2023 15:07

khiga8 added 7 commits March 3, 2023 10:10

use lines instead of source range

dfc78e2

add support for no_unused_disable linter

521d923

This commit adds support for the no_unused_disable linter. This can be configured in `erb-lint.yml`. If there are any unnecessary disable statements, these will be reported. This is a special linter that must run after all other linters.

Add specs

36cb987

update linter so tests pass

0af4d51

add test for no_unused_disable

80bfed6

run rubocop and move to private methods

7274bd8

khiga8 added 3 commits March 7, 2023 18:08

use plural name

c46d46e

update source_range_for_code helper and use it

6e644fd

add test for disable-inline-configs

9f95018

khiga8 commented Mar 7, 2023

View reviewed changes

spec/erb_lint/cli_spec.rb

end

end

context "with --disable-inline-configs" do

Copy link
Copy Markdown

Contributor Author

khiga8 Mar 7, 2023

Choose a reason for hiding this comment

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

I introduced a new config.

khiga8 marked this pull request as ready for review March 7, 2023 23:44

khiga8 requested review from jonrohan and rafaelfranca and removed request for rafaelfranca March 7, 2023 23:48

jonrohan approved these changes Mar 7, 2023

View reviewed changes

khiga8 requested a review from rafaelfranca March 15, 2023 23:06

rafaelfranca reviewed Mar 17, 2023

View reviewed changes

Move test helper to module

f52fc0f

khiga8 requested a review from rafaelfranca March 20, 2023 17:52

Use erblint:disable not erblint:disable-line to align with Rubocop

0afcd51

jonrohan approved these changes Mar 24, 2023

View reviewed changes

khiga8 merged commit fe0faef into Shopify:main Mar 24, 2023

khiga8 deleted the kh-allow_linter_disable_by_comment branch March 24, 2023 20:05

shopify-shipit bot temporarily deployed to production March 27, 2023 21:33 Inactive

khiga8 mentioned this pull request Mar 27, 2023

Migrate away from Counter system github/erblint-github#56

Closed

domingo2000 mentioned this pull request Sep 23, 2025

Linter: Allow disabling offenses using <%# herb:disable %> marcoroth/herb#531

Merged

asilano mentioned this pull request Oct 21, 2025

update to upstream fac/herb#13

Merged


		require "erb_lint/all"

		def source_range_for_code(processed_source, code)

Conversation

khiga8 commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Proposal

examples

Approach

What should reviewers focus on?

To do

Uh oh!

This comment was marked as outdated.

Uh oh!

koddsson left a comment

Choose a reason for hiding this comment

Uh oh!

rafaelfranca commented Feb 25, 2022

Uh oh!

khiga8 commented Feb 25, 2022

1. Comment on offending lines

2. Comment on previous line

Uh oh!

khiga8 commented Mar 8, 2022

Uh oh!

rafaelfranca commented Mar 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

khiga8 commented Mar 27, 2022

Uh oh!

This comment was marked as resolved.

khiga8 Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

khiga8 commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonrohan left a comment

Choose a reason for hiding this comment

Uh oh!

rafaelfranca Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

khiga8 Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

rafaelfranca commented Mar 17, 2023

Uh oh!

jonrohan commented Mar 17, 2023

Uh oh!

khiga8 commented Mar 20, 2023

Uh oh!

rafaelfranca commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonrohan commented Mar 24, 2023

Uh oh!

khiga8 commented Mar 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

khiga8 commented Feb 4, 2022 •

edited

Loading

khiga8 commented Mar 7, 2023 •

edited

Loading

rafaelfranca commented Mar 24, 2023 •

edited

Loading