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

--report-unused-disable-directives should be autofixable #11815

Closed
dmnd opened this issue Jun 7, 2019 · 15 comments · Fixed by #14617
Closed

--report-unused-disable-directives should be autofixable #11815

dmnd opened this issue Jun 7, 2019 · 15 comments · Fixed by #14617
Assignees
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 core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint needs design Important details about this change need to be discussed

Comments

@dmnd
Copy link

dmnd commented Jun 7, 2019

The version of ESLint you are using.
v5.16.0

The problem you want to solve.
I want to enable --report-unused-disable-directives on a large existing codebase, but it surfaces too many problems to be practical to fix manually. So I'd need to write a script. At that point, why not make the directive autofixable?

Your take on the correct solution to problem.
Run eslint --fix --report-unused-disable-directives

Are you willing to submit a pull request to implement this change?
Yes

@dmnd dmnd added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Jun 7, 2019
@mysticatea mysticatea added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jun 7, 2019
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Jul 8, 2019
@eslint-deprecated
Copy link

eslint-deprecated bot commented Jul 8, 2019

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@platinumazure
Copy link
Member

platinumazure commented Jul 8, 2019

Apologies for letting this slip through the cracks.

Personally, I'm not sure this should be fixable for the same reason that rules like no-unused-vars should not be fixable: We can't know if a user had typed a correct eslint-disable comment, and now it's no longer needed due to a change in the code; vs if a user had typed an incorrect eslint-disable comment (e.g., mistyped the rule name to be disabled) and it never worked, and so now we're just removing the comment and the user might not even be aware they had mistyped something.

That's just my opinion, though-- if other team members disagree and think this is worth implementing, I will defer to consensus.

@dmnd
Copy link
Author

dmnd commented Jul 9, 2019

@platinumazure can you kindly help me understand how a user could be unaware they mistyped something?

If the user mistyped a disable comment and it never worked, I think removing it is desirable, because either:

  1. The build is already broken, and the user will be prompted to add the disable comment again when CI fails
  2. The build isn't broken, and the disable comment is pointless and should be removed for readability

I suppose I'm assuming eslint users have builds fail when errors are reported. Is it common to use eslint differently?

@platinumazure
Copy link
Member

platinumazure commented Jul 9, 2019

@dmnd Thanks for your reply. To be honest, I hadn't thought into it very deeply and your reasoning makes sense. I think I could imagine myself being annoyed if I had my disable comment get nuked and I had to type it out again, just because of a typo, but I agree the impact is no worse than that.

@dmnd
Copy link
Author

dmnd commented Jul 10, 2019

@platinumazure also thanks to you for engaging in this issue ❤️

do you think eslint would be receptive to this idea? should the issue be opened again?

@platinumazure
Copy link
Member

platinumazure commented Jul 10, 2019

Looks like @mysticatea had left a 👍 on the issue. @mysticatea, would you like to reopen and champion this issue?

@mysticatea
Copy link
Member

mysticatea commented Jul 14, 2019

Thank you for the ping.

Yes, I will champion.

In most cases, people write disable comments to disable existing errors, different from no-unused-vars, so I think that autofix will not be annoying them.

@mysticatea mysticatea reopened this Jul 14, 2019
@mysticatea mysticatea removed the auto closed The bot closed this issue label Jul 14, 2019
@mysticatea mysticatea self-assigned this Jul 14, 2019
@dmnd dmnd changed the title --report-unused-disable-directive should be autofixable --report-unused-disable-directives should be autofixable Aug 19, 2019
@kaicataldo
Copy link
Member

kaicataldo commented Sep 29, 2019

@mysticatea Should we make an RFC for this?

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Sep 15, 2020

I'm interested tackling this issue & writing that RFC but I'm still a little shaky on the governance process here. Is this ready for me to send a PR to https://github.com/eslint/rfcs?

@aladdin-add aladdin-add self-assigned this Feb 2, 2021
@aladdin-add
Copy link
Member

aladdin-add commented Feb 2, 2021

seems a reasonable enhancement, I'll support it!

not sure an RFC is required - it's just a non-breaking enhancement. // cc @mdjermanovic

@nzakas nzakas added this to Needs Triage in Triage via automation Apr 8, 2021
@nzakas nzakas moved this from Needs Triage to Evaluating in Triage Apr 8, 2021
@nzakas
Copy link
Member

nzakas commented Apr 8, 2021

Is anyone still interested in this? It’s been open for over a year without any movement. Unless someone feels very strongly, we should close this as it was never accepted by the team.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Apr 8, 2021

FWIW I'd be very interested in tackling it soon, if the team wills it.

@aladdin-add
Copy link
Member

aladdin-add commented Apr 8, 2021

TSC Summary: This proposal proposes to make the --report-unused-disable-directives warnings fixable(runing eslint ---fix).

TSC Question: Shall we accept this proposal? Is a RFC required?

@aladdin-add aladdin-add added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Apr 8, 2021
@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion needs design Important details about this change need to be discussed and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Apr 11, 2021
@btmills btmills moved this from Evaluating to Waiting for RFC in Triage Apr 11, 2021
@btmills
Copy link
Member

btmills commented Apr 11, 2021

The team agreed that we'd like a way to autofix unused disable directives! Now we need an RFC to figure out how it should be implemented. So far we only autofix rules, so this will be a completely new type of autofixing. We're not sure that including this autofix in --fix is necessarily the right solution. Maybe we need a new CLI option, or maybe we can use --fix but with a new --fix-type value, but those are just ideas to get the brainstorm started.

@JoshuaKGoldberg thanks for your continued interest in working on this feature! As you're starting on the RFC, feel free to discuss anything or ask for early feedback on ideas in this issue, then we can move discussion to the RFC PR once you've opened one with a concrete proposal.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Apr 12, 2021

eslint/rfcs#78

@nzakas nzakas moved this from Waiting for RFC to RFC Opened in Triage Apr 13, 2021
@nzakas nzakas moved this from RFC Opened to Pull Request Opened in Triage May 25, 2021
@nzakas nzakas added this to Planned in v8.0.0 May 25, 2021
@nzakas nzakas linked a pull request May 25, 2021 that will close this issue
1 task
@mdjermanovic mdjermanovic moved this from Planned to Pull Request Opened in v8.0.0 May 26, 2021
Triage automation moved this from Pull Request Opened to Complete Aug 5, 2021
v8.0.0 automation moved this from Pull Request Opened to Done Aug 5, 2021
nzakas pushed a commit that referenced this issue Aug 5, 2021
* New: Fixable disable directives

* Part of the way there: not ready yet

* Generally fixed up, and just started on unit tests

* lodash.flatMap

* Merge branch 'master'

* Progress...

* Fixed the remaining pre-existing unit tests

* Fixed up some tests

* unprocessedDirective.parentComment

* Avoided rescan by passing commentToken in parentComment

* A lil edge case

* Fix directive grouping the other direction

* Incorrect state directive reference

* Simplified parentComment / commentToken range

* Use Map in lib/linter/apply-disable-directives.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Used suggestion for individual directives removal

* Boolean in lib/cli-engine/cli-engine.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Docs fixups

* Added tests

* Tests and fix for multiple rule lists

* Remove unused sourceCode from tests

* Added high-level test in linter.js

* Apply suggestions from code review

Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Added require

* Wrong order...

* Also mention in command-line-interface.md

* Respect disableFixes (not yet tested)

* Fixed up existing tests

* Add unit test for options.disableFixes

* Apply suggestions from code review

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>
@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 core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint needs design Important details about this change need to be discussed
Projects
Triage
Complete
v8.0.0
  
Done
Development

Successfully merging a pull request may close this issue.

8 participants