Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking: Fixable disable directives (fixes #11815) #14617

Merged
merged 33 commits into from Aug 5, 2021

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented May 23, 2021

Prerequisites checklist

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 eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label May 23, 2021
@eslint-github-bot
Copy link

eslint-github-bot bot commented May 23, 2021

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

@JoshuaKGoldberg JoshuaKGoldberg changed the title Fixable disable directives New: Fixable disable directives May 23, 2021
@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features and removed triage An ESLint team member will look at this issue soon labels May 25, 2021
@nzakas nzakas changed the title New: Fixable disable directives Breaking: Fixable disable directives May 25, 2021
@nzakas nzakas linked an issue May 25, 2021 that may be closed by this pull request
@nzakas nzakas changed the title Breaking: Fixable disable directives Breaking: Fixable disable directives (fixes #11815) May 25, 2021
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
@aladdin-add
Copy link
Member

aladdin-add commented Jun 14, 2021

friendly ping @JoshuaKGoldberg

@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Jun 14, 2021

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
Copy link
Contributor Author

JoshuaKGoldberg commented Jun 22, 2021

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.

Copy link
Member

@btmills btmills left a comment

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

lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
docs/user-guide/command-line-interface.md Show resolved Hide resolved
@nzakas
Copy link
Member

nzakas commented Jul 29, 2021

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

@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Jul 29, 2021

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.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

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

lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
tests/lib/linter/apply-disable-directives.js Outdated Show resolved Hide resolved
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

@TSMMark
Copy link

TSMMark commented Aug 3, 2021

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:

Screen Shot 2021-08-03 at 3 24 05 PM

Thanks again for your work on this

@mdjermanovic
Copy link
Member

mdjermanovic commented Aug 3, 2021

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

Screen Shot 2021-08-03 at 3 24 05 PM

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.

@nzakas nzakas merged commit 1d2213d into eslint:master Aug 5, 2021
12 checks passed
@JoshuaKGoldberg JoshuaKGoldberg deleted the fixable-disable-directives branch Aug 5, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 2, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--report-unused-disable-directives should be autofixable
7 participants