feat(eslint-plugin): create ban-ts-comment rule

[G-Rath]

Pretty much a clone of ban-ts-ignore - it might be nice to merge them into some form of ban-ts-control-comments rule 🤔

On a general aside, I'm interested in hearing what you've found better overall: lots of small nice rules, or a couple of big (but ideally still relatively straightforward in layout) rules, as it's a thought I'd had a couple of times as part of maintaining eslint-plugin-jest.

I think this could be worth having in the recommended list, w/ warn level.

[typescript-eslint]

Thanks for the PR, @G-Rath!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[bradzacher]

I'm interested in hearing what you've found better overall: lots of small nice rules, or a couple of big (but ideally still relatively straightforward in layout) rules

I don't think there's a hard-and-fast rule.
The thing I've learned from doing this is it depends on the use case.

Having one rule makes it easy to for a user to turn on everything at once, lets you deduplicate code, etc, but it comes with the drawback of only allowing a single level for the options - it's all an error, or it's all a warning.

Having two rules makes it harder for the user, as they have to review more docs, and it means you have to work harder to deduplicate code, etc, but it means that users can independently configure the level of each.

It entirely depends on the use case. If you think people will want individual levels, then two rules make sense. If you think people will not care, then one rule makes sense.

Examples:

For naming conventions, there is camelcase, class-name-casing, generic-type-naming, member-naming, etc. All of these rules are a pain to configure separately because they all have different options, and nobody will ever want different levels for the different naming sections, so it makes complete sense to merge them all into a single naming rule (#1318).

In the case of type assertions, in 2.0.0, I merged a number of rules into one rule (consistent-type-assertions). However this ran into the problem that people want to be able to both enforce that nobody uses angle bracket assertions, and nobody uses object type assertions. Because it's all one rule, someone might eslint-disable the single rule, which then means they are free to accidentally use an angle bracket assertion.
In that case, it was a mistake - there should be two separate rules.

In this example - it could go either way, it could be valid that someone might want error level for ban-ts-nocheck, as it has huge impact in disabling the entire file, but a warning for ban-ts-ignore, as it only disables a single line.

But I highly doubt anyone would want that.
I think this would make a lot sense together as a single rule like ban-ts-directive, with options for each of them (@ts-nocheck default true, @ts-ignore default true, @ts-check default false).

[bradzacher]

Following on from that big spiel, I think it makes sense to have a single rule for the @ts- directives.

Could you please merge this and ban-ts-ignore together, and deprecate ban-ts-ignore?

I'm thinking a simple object option:

interface Option {
  'ts-ignore'?: boolean;
  'ts-nocheck'?: boolean;
  'ts-check'?: boolean;
}

const defaultOptions: [Options] = [
  {
    'ts-ignore': true,
    'ts-nocheck': true,
    'ts-check': false,
  }
];

Naming wise, up to you - ban-ts-directive, ban-ts-directive-comment, ban-ts-comment, they're all fine.
I think I prefer the verbose but explicit ban-ts-directive-comment, but I'm not sold. IDK if people know what a directive is, so maybe just ban-ts-comment is better...

[G-Rath]

I don't think there's a hard-and-fast rule.
The thing I've learned from doing this is it depends on the use case.

Thanks for taking the time write up such a detailed response - it's pretty much the same as what I've been thinking in my head, but it's useful to hear independently aligns with a repo of this scale.

Could you please merge this and ban-ts-ignore together, and deprecate ban-ts-ignore?

Sure thing - I'll do that when I get home in a few hours :) (or tomorrow)

I think I prefer the simpler "comment" over "directive", just for ease of spelling & less technical.

[G-Rath]

I might have slightly forgotten about this 😬

I'll try and get it done over this weekend, so that it's ready in time for #1423

[bradzacher]

#1318 has landed, which included a fix for the recommended config generator.
So please re-run the generator and it'll create the correct config for 2.x.

With 3.0.0 we will remove ban-ts-ignore and add ban-ts-comment as a recommended.

One nit with naming - backticks are so much nicer.

Otherwise LGTM - thanks for doing this!

[bradzacher]

woops was supposed to RC sorry

[bradzacher]

LGTM! Great work