Skip to content

feat: add ignoreDirectives option in no-unused-expressions #19645

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

Merged
merged 6 commits into from
Apr 24, 2025

Conversation

sethamus
Copy link
Contributor

@sethamus sethamus commented Apr 21, 2025

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

What changes did you make? (Give an overview)

This PR adds a new ignoreDirectives option to the no-unused-expressions rule to address compatibility issues with ES3 codebases.

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

@sethamus sethamus requested a review from a team as a code owner April 21, 2025 23:32
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Apr 21, 2025
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Apr 21, 2025
@github-actions github-actions bot added the rule Relates to ESLint's core rules label Apr 21, 2025
Copy link

netlify bot commented Apr 21, 2025

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit c926980
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6809353b9233620008f46538

@snitin315
Copy link
Contributor

snitin315 commented Apr 21, 2025

I had already raised #19644 😅

@sethamus
Copy link
Contributor Author

Oh shoot, I didn't see that! Mine is a bit more elaborate; would you like to go with my PR?

@snitin315
Copy link
Contributor

I think we would want ignoreDirective option to ignore all directive nodes regardless of ecmaversion, that way we can just rely on the astUtils.isDirective only and remove the unwanted code from the rule

@sethamus
Copy link
Contributor Author

sethamus commented Apr 22, 2025

Oh ok I removed those code. All tests seem to pass.

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion contributor pool labels Apr 22, 2025
@mdjermanovic mdjermanovic moved this from Needs Triage to Implementing in Triage Apr 23, 2025
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.

Just one more request, then LGTM.

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! Leaving open for @snitin315 to verify.

Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@snitin315 snitin315 merged commit 2dfd83e into eslint:main Apr 24, 2025
32 checks passed
@github-project-automation github-project-automation bot moved this from Implementing to Complete in Triage Apr 24, 2025
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 contributor pool feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

3 participants