[Feature] Allow rule to be disabled by comment #249

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.

lib/erb_lint/cli.rb

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

spec/erb_lint/cli_spec.rb

@@ -98,6 +98,33 @@ def run(processed_source)
      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

@@ -1,3 +1,8 @@
 # frozen_string_literal: true

 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.

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 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

lib/erb_lint/runner.rb Outdated Show resolved Hide resolved

lib/erb_lint/runner.rb Show resolved Hide resolved

lib/erb_lint/runner.rb Outdated Show resolved Hide resolved

lib/erb_lint/linter.rb Outdated Show resolved Hide resolved

lib/erb_lint/linter.rb Outdated Show resolved Hide resolved

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 2 commits March 6, 2023 22:49

Introduce new spec helper to more easily predict source range

ab1ab6a

swap with new spec helper

0b460a8

khiga8 commented Mar 7, 2023

View reviewed changes

lib/erb_lint/cli.rb Outdated Show resolved Hide resolved

khiga8 added 4 commits March 7, 2023 17:56

rename spec

a9c799e

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

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

khiga8 requested review from rafaelfranca and jonrohan 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Allow rule to be disabled by comment #249

[Feature] Allow rule to be disabled by comment #249

khiga8 commented Feb 4, 2022 •

edited

Loading

This comment was marked as outdated.

koddsson left a comment

rafaelfranca commented Feb 25, 2022

khiga8 commented Feb 25, 2022

khiga8 commented Mar 8, 2022

rafaelfranca commented Mar 16, 2022

khiga8 commented Mar 27, 2022

This comment was marked as resolved.

khiga8 Mar 7, 2023

khiga8 commented Mar 7, 2023 •

edited

Loading

jonrohan left a comment

rafaelfranca Mar 17, 2023

khiga8 Mar 17, 2023

rafaelfranca commented Mar 17, 2023

jonrohan commented Mar 17, 2023

khiga8 commented Mar 20, 2023

rafaelfranca commented Mar 24, 2023 •

edited

Loading

jonrohan commented Mar 24, 2023

khiga8 commented Mar 24, 2023

[Feature] Allow rule to be disabled by comment #249

[Feature] Allow rule to be disabled by comment #249

Conversation

khiga8 commented Feb 4, 2022 • edited Loading

Context

Proposal

examples

Approach

What should reviewers focus on?

To do

This comment was marked as outdated.

koddsson left a comment

Choose a reason for hiding this comment

rafaelfranca commented Feb 25, 2022

khiga8 commented Feb 25, 2022

1. Comment on offending lines

2. Comment on previous line

khiga8 commented Mar 8, 2022

rafaelfranca commented Mar 16, 2022

khiga8 commented Mar 27, 2022

This comment was marked as resolved.

khiga8 Mar 7, 2023

Choose a reason for hiding this comment

khiga8 commented Mar 7, 2023 • edited Loading

jonrohan left a comment

Choose a reason for hiding this comment

rafaelfranca Mar 17, 2023

Choose a reason for hiding this comment

khiga8 Mar 17, 2023

Choose a reason for hiding this comment

rafaelfranca commented Mar 17, 2023

jonrohan commented Mar 17, 2023

khiga8 commented Mar 20, 2023

rafaelfranca commented Mar 24, 2023 • edited Loading

jonrohan commented Mar 24, 2023

khiga8 commented Mar 24, 2023

khiga8 commented Feb 4, 2022 •

edited

Loading

khiga8 commented Mar 7, 2023 •

edited

Loading

rafaelfranca commented Mar 24, 2023 •

edited

Loading