Tslint: Reasons/descriptions no longer work in disable all rule lines

[dhedey]

Bug Report

• TSLint version: 3.14.0
• TypeScript version: 1.8.10
• Running TSLint via: grunt-tslint
  

  TypeScript code being linted

// tslint:disable-next-line - Because I feel like a rebel, and I never liked === anyway
if (a == b) {

tslint.json

{
    "rules": {
        "triple-equals": true
    }
}

Actual behavior

TSLint triggers on the next line

Expected behavior

TSLint does not trigger on the next line (for any rule)

Extra Information

As of #1352 (fixing #1269), there have been some incompatibilities with how our team would like to use tslint. In our coding guidelines, whenever linting is disabled (and we usually do a one line disable using disable-next-line), we think it good practice to add an explanation of why we have overridden the rule. We typically add this comment in the same line as the disable, but this now no longer works.

In particular, #1352 changed it so that (even without a second semi-colon in a line), it tries to parse each 'word' of the comment into a rule.

This is possibly a DNF, but I thought it was worth a discussion. In particular, is it worth adding a syntax to enable descriptions (ie non-rules) in disable lines?

I'd personally advocate something like "all text after a single dash is a comment", but I don't know if anyone has any thoughts on this?

Workarounds exist, but in my opinion aren't as nice:

• You could specify the rule's code, to ignore explicitly
  
  
  
  • In VS Code (the IDE we're currently using), this isn't given in the error message, so it's a bit of a faff to find it (arguably something I should raise with the VS Code extension, but that's a separate question).
  
  
  • Also, the words of the description are still parsed as rules, which is not the intention.
  
  
• Add a comment on the preceding line with the reason
  
  
  
  • It would imo be nicer if the following line worked, but then the disable-next-line disables the comment
  
  

[adidahiya]

Ah, good catch @dhedey. I like the idea of specifying a reason for disabling rules. Stylelint actually has a rule enforcing this which I'd like to port over to TSLint -- it expects an explanatory comment on the line above a disable comment. I'm inclined to go in that direction as the pattern for explanatory comments (instead of an inline explanation) for consistency's sake, but I'd like to hear others' opinions as well.

[glen-84]

I often use something like:

// tslint:disable:no-any -- We have no option here, 3rd-party code.

The enforcing rule could check above, below, and "inline" for a comment.

[dhedey]

Thanks for the speedy response and info / links @adidahiya. That sounds like a reasonable suggestion!

Personally, I find that a two line block to ignore an overly prescriptive linting rule can begin to affect readability... but it's hardly a big problem!

So whilst I would still lend my support to some comment-in-line syntax on top of this (and would be happy to create a pull request for a proposed solution), I can appreciate that a lack of consistency with other linters may be a drawback.

Might be interesting to hear if anyone else has any thoughts on this.

[JoshuaKGoldberg]

I like this a lot 😊 and find myself asking _"why?"_ for disables in pull requests where the answers often end up being misconceptions about TypeScript or TSLint. +1!

How about two _(breaking)_ changes:

• Only allowing rule names separated with :
• A rule that flags tslint:disable*s without a properly formatted reason

[glen-84]

I don't think that it's necessary to introduce a breaking change, if you use one or more characters that would not be found in a rule name. Examples:

• -- (also used by PHP Code Sniffer)
  
  
  
  • // tslint:disable:rule1 rule2 rule3 -- Reasons.
  
  
• *
  
  
  
  • // tslint:disable:rule1 rule2 rule3 * Reasons.
  
  
• /
  
  
  
  • // tslint:disable:rule1 rule2 rule3 / Reasons.
  
  
• (...)
  
  
  
  • // tslint:disable:rule1 rule2 rule3 (Reasons)
  
  
• etc.

A single hyphen might be possible as well, if you require whitespace around it, or disallow rule names from starting or ending with a hyphen.

[JoshuaKGoldberg]

if you use one or more characters that would not be found in a rule name

That unfortunately would be breaking in the case of folks doing something like this:

// tslint:disable-next-line:rule-name because reasons -- and other reasons --

Right now, that would disable any rule named because, reasons, and, or other, right? And after this change it would no longer?

[glen-84]

Technically, probably, but it would be really strange if someone was relying on that. 🙂

[crush83]

I'd really like to require reasons on tslint disables as well. Has there been any progress towards making a rule that can enforce that a reason is given on a tslint disable?

Is //tslint:disable -- reason the accepted format?

[JoshuaKGoldberg]

💀 _It's time!_ 💀

TSLint is being deprecated and no longer accepting pull requests for major new changes or features. See #4534. 😱

If you'd like to see this change implemented, you have two choices:

• Recommended: Check if this is available in ESLint + typescript-eslint ✅
• _Not Recommended: Fork TSLint locally_ 🤷‍♂️

👋 It was a pleasure open sourcing with you!

_If you believe this message was posted here in error, please comment so we can re-open the issue!_

[JoshuaKGoldberg]

🤖 Beep boop! 👉 TSLint is deprecated 👈 _(#4534)_ and you should switch to typescript-eslint! 🤖

🔒 This issue is being locked to prevent further unnecessary discussions. Thank you! 👋