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

[no-sequences] Add option to disable the "in-parentheses" exception #14197

Closed
danielrentz opened this issue Mar 10, 2021 · 4 comments · Fixed by #14199
Closed

[no-sequences] Add option to disable the "in-parentheses" exception #14197

danielrentz opened this issue Mar 10, 2021 · 4 comments · Fixed by #14199
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects

Comments

@danielrentz
Copy link
Contributor

danielrentz commented Mar 10, 2021

Follow-up for bug report #14184

What rule do you want to change?
no-sequences

Does this change cause the rule to produce more or fewer warnings?
more warnings

How will the change be implemented? (New option, new default behavior, etc.)?

  • New option that disables (or enables) the exception that sequences are allowed in parentheses.

Please provide some example code that this change will affect:

if ((x1 === x2) && (y1, y2)) {

Intended was

if ((x1 === x2) && (y1 === y2)) {

This real-life bug sneaked in after replacing a comparison function with the comparison operator

- if (isEqual(x1, x2) && isEqual(y1, y2)) {
+ if ((x1 === x2) && (y1, y2)) {

It was hard to find because the sequence expression was hidden in a short-circuiting AND operator.

What does the rule currently do for this code?
Lets it pass.

What will the rule do after it's changed?
Warns for the sequence.

Are you willing to submit a pull request to implement this change?
No. Yes.

@danielrentz danielrentz added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Mar 10, 2021
@mdjermanovic mdjermanovic removed the triage An ESLint team member will look at this issue soon label Mar 10, 2021
@mdjermanovic mdjermanovic added this to Needs Triage in Triage via automation Mar 10, 2021
@mdjermanovic mdjermanovic moved this from Needs Triage to Evaluating in Triage Mar 10, 2021
@mdjermanovic
Copy link
Member

Thanks for opening this new issue!

New option that disables (or enables) the exception that sequences are allowed in parentheses.

The proposed option makes sense to me 👍

@aladdin-add
Copy link
Member

@danielrentz no worries, I will champion the change. :)

it may take a while for the issue being reviewed by other team members, and reach consensus.

@aladdin-add aladdin-add self-assigned this Mar 12, 2021
@nzakas
Copy link
Member

nzakas commented Mar 23, 2021

Makes sense to me. 👍

@btmills
Copy link
Member

btmills commented Mar 24, 2021

Agreed 👍 and marking as accepted.

@btmills btmills added the accepted There is consensus among the team that this change meets the criteria for inclusion label Mar 24, 2021
@btmills btmills moved this from Evaluating to Pull Request Opened in Triage Mar 24, 2021
Triage automation moved this from Pull Request Opened to Complete Mar 26, 2021
mdjermanovic pushed a commit that referenced this issue Mar 26, 2021
#14199)

* New: add option "allowInParentheses" to rule "no-sequences"

* added documentation

* [no-equence]: switch default of "allowInParentheses" to true

* restored removed sentence

* changes from code review

* code review
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Sep 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

5 participants