-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
I had already raised #19644 😅 |
Oh shoot, I didn't see that! Mine is a bit more elaborate; would you like to go with my PR? |
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 |
Oh ok I removed those code. All tests seem to pass. |
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
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 theno-unused-expressions
rule to address compatibility issues with ES3 codebases.Is there anything you'd like reviewers to focus on?