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

ESLint Plugin: Add rule no-unjustified-disable #16795

Closed
wants to merge 1 commit into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 29, 2019

This pull request seeks to add a new ESLint rule which enforces that the disabling of an ESLint rule via inline comment configuration must be accompanied by a preceding comment which justifies said disabling, using the "Disable reason:" convention already present in many occurrences in the codebase.

The reason for this rule to exist is explained in the included rule README:

If a developer finds themself in the position to disable an ESLint rule, one of the following should hold true:

  • They can provide a reason the rule is not applicable or remediation steps are provided.
  • The rule does not provide value for the development team.

By providing justification for the disabling of a rule, a developer communicates intent to future maintainers who may otherwise not understand the reasons it was done. Such uncertainty can needlessly discourage refactoring or unnecessarily perpetuate the configuration. Good justification can serve as a description for the conditions of its own removal.

If a rule is not valuable, it should not be part of the development team's default configuration.

Status:

The rule itself is considered complete. However, there are a large number (128) of existing problems which must first be addressed.

Testing Instructions:

Verify there are no lint errors:

npm run lint-js

Verify unit tests pass:

npm run test-unit packages/eslint-plugin/rules/__tests__/no-unjustified-disable.js

@aduth
Copy link
Member Author

aduth commented May 11, 2020

Relevant: https://eslint.org/blog/2020/02/whats-coming-in-eslint-7.0.0#descriptions-in-directive-comments

ESLint now supports comments as part of inline configuration, as of the latest 7.0.0 release. It's relevant for this pull request, in that it should be at the very least support—if not preferred—to document justifications this way, since it appears to be the primary use-case for the feature.

@chrisvanpatten
Copy link
Member

I love this rule and would 100% use it!

Base automatically changed from master to trunk March 1, 2021 15:42
@gziolo gziolo added Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code and removed Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels May 7, 2022
@gziolo gziolo added this to Needs review in Core JS May 7, 2022
@aduth
Copy link
Member Author

aduth commented May 27, 2022

This could still be useful, but the pull request itself is very stale, and I don't know that it's still a pressing concern for the project.

@aduth aduth closed this May 27, 2022
Core JS automation moved this from Needs review to Done May 27, 2022
@aduth aduth deleted the add/eslint-no-unjustified-disable branch May 27, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] ESLint plugin /packages/eslint-plugin [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Core JS
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants