Linter: Ability to opt out of Herb 0.9 linting rules based on better-html and erb_lint

[mudge]

We've been happily using @herb-tools/linter to check the correctness of our ERB templates in several Rails applications (e.g. unbalanced tags, invalid/missing attributes, etc.). However, the upgrade to Herb 0.9 has caused a large number of warnings and errors due to the new linting rules.

We don't necessarily agree with some of these rules, e.g. aggressively trying to prevent duplicate branch elements even if the repetition across conditional branches would be easier to read, preferring <script> over javascript_tag even if the latter automatically populates the nonce attributes, etc.

We do want to keep up-to-date with the latest version of the library and so are working around this by committing a .herb.yml generated by herb-lint --init on 0.8 so we default to opting out of the more opinionated rules.

Is there some balance here where use of herb-lint is more akin to Standard Ruby than Rubocop? Perhaps some classification of rules that lets us easily distinguish core "correctness" from more opinionated "style" rules?

[markokajzer]

I agree that a classification would help opt-in / opt-out of a whole set of rules, similar to how rubocop does it with Rubocop/Performance for example. i saw the mention of "herb plugins", like for turbo and a11y, across a few issues in the past, so maybe that was ultimately the idea from the beginning?

for the time being, i know it's tedious, but you can disable rules on a per-rule basis via .herb.yml, no need to limit yourself to an old set of rules. for example:

rules: 
  actionview-no-silent-helpers:
    enabled: false

[marcoroth]

It's a fair point and has been one of my biggest concerns with the linter, so thanks for raising this @mudge!

The linter has been overly aggressive on purpose and has had all its linter rules enabled by default so that these linter rules get seen and used by people. But at the same time I think we also need to tone down some of the more opinionated rules. We also haven't really balanced what rules are reporting as error and which ones as warning.

In a way, we also have been "abusing" the medium of linter rules to warn about some things that aren't necessarily offenses, like the "empty ERB blocks" or the "duplicate branch elements" elements you mentioned. They don't really let a build fail as they are reporting as "hints", but from a technical point of view it was the easiest way to just implement them as linter rules and have them report on a different offense level.

I'm really open to structure this in the way so that the Linter doesn't feel annoying. I don't want the linter to be a burden but a tool that can help you write better views and better view code in general. I'm also not sure how we should balance what a linter rule is and what should be "helpful diagnostics you'd want to see in your editor but that shouldn't let the build fail".

As @markokajzer mentioned, introducing some kind of groups would probably make sense, like security, style, formatting, performance, similar to RuboCop, but maybe also that you can say disable all actionview-* rules etc.

If you have any concrete ideas or opinions I'd love to hear them 🙌🏼

[mudge]

I'm really open to structure this in the way so that the Linter doesn't feel annoying. I don't want the linter to be a burden but a tool that can help you write better views and better view code in general. I'm also not sure how we should balance what a linter rule is and what should be "helpful diagnostics you'd want to see in your editor but that shouldn't let the build fail".

Yeah, I think we felt this keenly because we use Herb as a mandatory linting step in CI so our exposure to the new rules was in the form of all builds suddenly breaking and restricting our ability to deploy. That's obviously the worse case but I don't think is atypical (we also run the aforementioned Standard as well as Brakeman, Biome, etc.)

I agree there is a separate use case here which is for Herb to be providing guidance whilst editing but perhaps the split here is less about category and more about severity?

[maxim]

I started using herb for linting just a week ago and the potential is great, but in practice it's been a bit messy. I'm unclear why there's a gem and npm version, what and how I'm supposed to use. I figured npm catches a lot more, so I moved to using npx @herb-tools/linter over herb analyze, but not sure if that's what's recommended.

But besides that, it's rules like the one discussed here.

I don't want to disable erb-no-duplicate-branch-elements completely, but I do want it to tell me legit "here's a tag that can be simply moved out of a condition" situations. I also love knowing that there's a useless empty erb tag or class="". Those are clear and fair flags.

I just don't like when it flags stuff like the following.

<% if @model.active? %>
  <div class="col-auto">
    <%= button_to # … code … %>
  </div>
<% else %>
  <div class="col-auto" <%= tip_attrs('Already deactivated.', :left) %>>
    <%= button_to # … other code … %>
  </div>
<% end %>

In the above, it complains that <div class="col-auto"> is the same, whereas it's clearly not. Can I disable this specific part of erb-no-duplicate-branch-elements, and not the whole thing?

In linters like rubocop they allow some config on specific rules. There could be a config on this rule, say detect_erb: false (default) or ignore_erb: true (default) or something like that.