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

feat!: flexible config + default reporting of unused disable directives #100

Merged
merged 14 commits into from
Jul 21, 2023

Conversation

bmish
Copy link
Member

@bmish bmish commented Dec 9, 2022

Summary

Provide users with more flexible, intuitive control over how unused disable directives are reported by supporting severity levels instead of just boolean values with CLI and config file options. Change the default behavior from off to warn on unused disable directives.

Related Issues

Draft implementation of phase 1 of this RFC (the non-breaking changes):

The issue triggering this RFC:

Other related issues:

@eslint-github-bot
Copy link

Hi @bmish!, 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 length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

@bmish bmish force-pushed the unused-disable-directive-config branch from 6783236 to c5b1c10 Compare December 9, 2022 19:01
@bmish bmish changed the title feat!: flexible configuration and stricter default behavior for reporting unused disable directives feat!: flexible config + default reporting of unused disable directives Dec 9, 2022
@bmish bmish force-pushed the unused-disable-directive-config branch from c5b1c10 to 132158c Compare December 9, 2022 19:02
@nzakas nzakas added the Initial Commenting This RFC is in the initial feedback stage label Dec 9, 2022
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together. Overall, I think have the option on by default makes sense, but I think this plan is a bit too disruptive as described. I think we can make it less disruptive by continuing to support boolean settings forever and by having the default be warn instead of error.

@bmish bmish force-pushed the unused-disable-directive-config branch 3 times, most recently from 94ef555 to bb6df4e Compare December 29, 2022 18:06
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we've come to a pretty good agreement about how to proceed here. We just need the RFC to be updated to reflect that agreement along with some other small points of clarification.

@nzakas
Copy link
Member

nzakas commented Mar 8, 2023

@bmish are you still working on this?

@nzakas
Copy link
Member

nzakas commented Apr 7, 2023

@bmish friendly ping. Do you intend to finish up this RFC?

@bmish
Copy link
Member Author

bmish commented Apr 10, 2023

Sorry for delay, will finish this up when I have a chance.

@bmish
Copy link
Member Author

bmish commented May 21, 2023

I have updated the RFC to incorporate all feedback so far.

@bmish
Copy link
Member Author

bmish commented May 23, 2023

I have opened up a draft PR of phase 1 (the non-breaking changes) from this RFC. It's fully working, tested, and documented, so this helps prove out the RFC and demonstrates what changes will be necessary.

@bmish bmish force-pushed the unused-disable-directive-config branch from 6a1d1ad to d8e3de7 Compare May 24, 2023 00:13
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I feel comfortable with this direction

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Moving to final commenting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking feature Final Commenting This RFC is in the final week of commenting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants