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: emit warning when .eslintignore file is detected #17952

Merged
merged 9 commits into from Jan 19, 2024

Conversation

snitin315
Copy link
Contributor

@snitin315 snitin315 commented Jan 4, 2024

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 autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

What changes did you make? (Give an overview)

Fixes #17831

Is there anything you'd like reviewers to focus on?

Now a lot of warnings are polluting the test command output

Screenshot 2024-01-04 at 4 37 11 PM

@snitin315 snitin315 requested a review from a team as a code owner January 4, 2024 10:17
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Jan 4, 2024
Copy link

netlify bot commented Jan 4, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit fc7c7ac
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/65a9e007fbfeaf0008661fbd

@snitin315 snitin315 marked this pull request as draft January 4, 2024 10:25
@snitin315 snitin315 marked this pull request as ready for review January 4, 2024 11:11
@mdjermanovic
Copy link
Member

Now a lot of warnings are polluting the test command output

Can we maybe move the patterns into ignorePatterns in the config file and remove the .eslintignore file?

@nzakas
Copy link
Member

nzakas commented Jan 4, 2024

I'm pretty sure there are tests that rely on .eslintrc.js and .eslintignore, which is why they're still present in the repo even though we aren't using them as part of our dev setup.

@mdjermanovic
Copy link
Member

I'm pretty sure there are tests that rely on .eslintrc.js and .eslintignore, which is why they're still present in the repo even though we aren't using them as part of our dev setup.

In #17204, we updated tests to not rely on this project's eslint config files. I'm not 100% sure about the .eslintignore file and if some new tests didn't slip in in the meantime, but we could try.

lib/eslint/eslint.js Outdated Show resolved Hide resolved
tests/bin/eslint.js Outdated Show resolved Hide resolved
Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>
@snitin315
Copy link
Contributor Author

Can we maybe move the patterns into ignorePatterns in the config file and remove the .eslintignore file?

I'll try this and see how much we can reduce such warnings.

@snitin315
Copy link
Contributor Author

I've removed the root .eslintignore, and the warning logs have been reduced by ~90. But there are still warnings from tests relying on the presence of test/fixtures/.eslintignore & --ignore-path which doesn't work with flat config. Should we remove/migrate these tests too?

@nzakas
Copy link
Member

nzakas commented Jan 5, 2024

We still need to have some tests that rely on .eslintignore because we are still supporting that file in eslintrc mode in v9.x. (In theory, we could remove them, but then we wouldn't know if we broke eslintrc mode in some way. I don't think that's a good idea.)

@mdjermanovic
Copy link
Member

We could suppress these warnings in test files that are producing them, for example this way:

sinon.stub(process, "emitWarning").withArgs(sinon.match.any, "ESLintIgnoreWarning").returns();
process.emitWarning.callThrough();

@snitin315
Copy link
Contributor Author

I've updated the corresponding tests to suppress the warnings.

@mdjermanovic
Copy link
Member

Now that we have found a way to suppress the warnings while running tests, what do you think about restoring the .eslintignore file for now? I ask because it turns out that over a hundred tests used this file, and while it appears that they're passing without it, it doesn't necessarily mean that they're still testing what they were intended to, so it might be good that we check each of them at some future point, and then remove the .eslintignore file.

@snitin315
Copy link
Contributor Author

snitin315 commented Jan 8, 2024

That makes sense, I think we can restore the .eslintignore file for now.

@snitin315
Copy link
Contributor Author

Done!

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. Would like @mdjermanovic to verify before merging.

Copy link
Sponsor Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for picking this up.

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels Jan 18, 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, just two small suggestions about the tests.

tests/lib/eslint/eslint.js Outdated Show resolved Hide resolved
tests/lib/cli-engine/file-enumerator.js Outdated Show resolved Hide resolved
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!

@mdjermanovic mdjermanovic merged commit e3051be into main Jan 19, 2024
17 checks passed
@mdjermanovic mdjermanovic deleted the throw-warning-eslintignore branch January 19, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Change Request: Warn when .eslintignore detected with flat config
4 participants