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: Reporting unused inline configs #121

Merged
merged 15 commits into from
Sep 5, 2024

Conversation

JoshuaKGoldberg
Copy link
Contributor

Summary

Proposes adding a CLI option to report /* eslint... */ comments that don't change any settings.

Related Issues

@fasttime fasttime added the Initial Commenting This RFC is in the initial feedback stage label Jul 8, 2024
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as draft July 9, 2024 18:48
@JoshuaKGoldberg
Copy link
Contributor Author

Per eslint/eslint#15476 (comment), converting to a draft so I can rework this to encompass eslint/eslint#15476.


[eslint/eslint#15476 Change Request: report unnecessary config overrides](https://github.com/eslint/eslint/issues/15476) previously suggested also checking config files (`eslint.config.*`).
Doing so could be beneficial to flag values that become unnecessary in config files over time.
However, because flat config purely involves spreading objects, there's no way to know what objects originate from shared configs or shared packages.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI @ljharb. I really wanted to get this support in, but just don't see a way to get it to work in this major version. 😞

Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably something better located in the config inspector, where we could give people a visualization of duplicate configs.

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review August 14, 2024 03:35
nzakas
nzakas previously approved these changes Aug 20, 2024
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.

This looks like a really solid proposal to me. I left a few comments for clarifications but overall I like the direction of this RFC so happy to approve it.


[eslint/eslint#15476 Change Request: report unnecessary config overrides](https://github.com/eslint/eslint/issues/15476) previously suggested also checking config files (`eslint.config.*`).
Doing so could be beneficial to flag values that become unnecessary in config files over time.
However, because flat config purely involves spreading objects, there's no way to know what objects originate from shared configs or shared packages.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably something better located in the config inspector, where we could give people a visualization of duplicate configs.

Adding a sole `--report-unused-inline-configs` CLI option presents a discrepency between the two sets of options.
An alternative could be to instead add `--report-unused-inline-configs` and `--report-unused-inline-configs-severity` options for consistency's sake.

This RFC's opinion is that the consistency of adding two new options is not worth the excess options logic.
Copy link
Member

Choose a reason for hiding this comment

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

Agreed. 👍

designs/2024-report-unused-inline-configs/README.md Outdated Show resolved Hide resolved
### Separate CLI Option

Unlike the changes discussed in [Change Request: Enable reportUnusedDisableDirectives config option by default](https://github.com/eslint/eslint/issues/15466) -> [feat!: flexible config + default reporting of unused disable directives](https://github.com/eslint/rfcs/pull/100), reporting unused inline configs does not have legacy behavior to keep to.
The existing `--report-unused-disable-directives` (enabling) and `--report-unused-disable-directives-severity` (severity) options were kept separate for backwards compatibility.
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, --report-unused-disable-directives is the same as --report-unused-disable-directives-severity error, and it isn't necessary to pass --report-unused-disable-directives to enable reporting when --report-unused-disable-directives-severity is used. In fact, these two options cannot be used together.

https://github.com/eslint/eslint/blob/d1f0831bac173fe3e6e81ff95c5abdbf95b02b65/lib/cli.js#L484-L487

mdjermanovic
mdjermanovic previously approved these changes Aug 28, 2024
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.

Looks good to me, thanks!

nzakas
nzakas previously approved these changes Aug 28, 2024
@nzakas
Copy link
Member

nzakas commented Aug 28, 2024

We have two approvals so moving to final commenting.

@nzakas nzakas added Final Commenting This RFC is in the final week of commenting and removed Initial Commenting This RFC is in the initial feedback stage labels Aug 28, 2024
Copy link
Member

@fasttime fasttime 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 this RFC! Just a question, then LGTM.

@nzakas
Copy link
Member

nzakas commented Sep 3, 2024

Just needs to stay open for one more day to finish out the final commenting period, then we're ready to merge.

@nzakas nzakas merged commit 860d5f4 into eslint:main Sep 5, 2024
3 checks passed
@JoshuaKGoldberg JoshuaKGoldberg deleted the reporting-unused-inline-configs branch September 5, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants