Breaking: Fixable disable directives (fixes #11815)

[JoshuaKGoldberg]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Implements eslint/rfcs#78 as described.

Is there anything you'd like reviewers to focus on?

I'm not confident I got all the places to update docs about the addition to fixTypes and friends... Thanks!

Would you like me to split out the added logic in apply-disable-directives.js into a new file and individually unit test it? I was dubious that re-searching through the source file for the previously retrieved disable directives is the right approach for performance, so I held off in the initial commit. I ended up storing a parentComment in linter.js > createDisableDirectives, to hold off on re-scanning.

[eslint-github-bot]

Hi @JoshuaKGoldberg!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag must be one of the following:

  The Tag is one of the following:

  • Fix - for a bug fix.
  • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
  • New - implements a new feature.
  • Breaking - for a backwards-incompatible enhancement or feature.
  • Docs - changes to documentation only.
  • Build - changes to build process only.
  • Upgrade - for a dependency upgrade.
  • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

  You can use the labels of the issue you are working on to determine the best tag.

• There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

[aladdin-add]

friendly ping @JoshuaKGoldberg

[JoshuaKGoldberg]

Hey thanks for the reviews and the ping! I'm about halfway through resolving the feedback locally and hope to finish within this week :)

[JoshuaKGoldberg]

Status update: I've added a "grouping" flow so that the logic knows whether an entire comment should be removed rather than only some of the rule IDs within. I ran out of time today to update the existing unit tests and hope to get to that soon, after which I'll touch up some of the mentioned edge cases around commas and spaces.

[btmills]

I’m still working my way through tests, but here’s a few quick fixes.

[nzakas]

@JoshuaKGoldberg just checking in to see if you're still working on this?

[JoshuaKGoldberg]

Sorry, I am - I missed the notification for the most recent round of reviews. Thanks for the ping (again)! I'll have them fixed up by the end of this weekend.

[mdjermanovic]

Looks great, I have only one suggestion about the fix property.

[mdjermanovic]

LGTM, thanks!

[TSMMark]

Tested this branch on a medium sized codebase, and it seems to work beautifully. So far I've only noticed one minor issue:

I don't know if this is in-scope, or if there's another lint rule that will report/fix this, but I noticed when eslint-disable rules are removed, there's a possibility to end up with multiple adjacent spaces in the disable comment.

Example:

Thanks again for your work on this

[mdjermanovic]

@TSMMark thanks for testing, and glad it works well for you!

I don't know if this is in-scope, or if there's another lint rule that will report/fix this, but I noticed when eslint-disable rules are removed, there's a possibility to end up with multiple adjacent spaces in the disable comment.

Example:

It's always difficult to produce a fix with the right formatting. We're usually making a minimal change that fixes the reported problem, and then rely on other rules to fix the whitespace afterward. Unfortunately, in this case there is no such rule. Since this is a minor issue that doesn't affect the correctness of this feature, and the solution doesn't seem trivial, I wouldn't address it in this PR (which is already fairly complex). If you open a separate issue, someone might be able to tweak the fix in another PR.