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

Update: add options to (dis)allow comma expression in for-loops in no-sequences #14204

Closed
danielrentz opened this issue Mar 12, 2021 · 3 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects

Comments

@danielrentz
Copy link
Contributor

What rule do you want to change?

no-sequences

This is a follow-up of #14197 (Add option to disable the "in-parentheses" exception).
Consequently, the rule should contain an option to decide whether commas are allowed in for-loop heads.

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

More.

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

Currently, comma expressions are always allowed in for-loop init and update part, but not in test part where it does not make any sense.

I have two alternatives in mind:

  1. One option for init and update part of the for-loop, e.g. "allowInForLoop". But this is confusing as it obfuscates the fact that comma will still not be allowed in test part.
  2. Distinct options for init and update part:
    • allowForLoopInit (following "ignoreForLoopInit" from init-declarations)
    • allowForLoopAfterthoughts (as in no-plusplus) (or allowForLoopUpdate which is shorter and better remarkable?)

All options will be true by default for compatibility.

Please provide some example code that this change will affect:

let i, j;
for (i = 0, j = 0; i < 100; i += 1, j += 2) {
  
}

What does the rule currently do for this code?

Passes.

What will the rule do after it's changed?

Pass or warn according to options.

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

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 12, 2021
@danielrentz danielrentz changed the title [no-sequences] new options to (dis)allow comma expression in for-loop Update: add options to (dis)allow comma expression in for-loops in no-sequences Mar 12, 2021
@mdjermanovic mdjermanovic removed the triage An ESLint team member will look at this issue soon label Mar 15, 2021
@mdjermanovic mdjermanovic added this to Needs Triage in Triage via automation Mar 15, 2021
@mdjermanovic mdjermanovic moved this from Needs Triage to Evaluating in Triage Mar 15, 2021
@mdjermanovic
Copy link
Member

I'm not much in favor of this option, as it's mostly stylistic. Comma in a for-loop init/update doesn't look like a possible error or a bad practice. It's also easy to disallow this with no-restricted-syntax, the selector would be:

"ForStatement > SequenceExpression.init, ForStatement > SequenceExpression.update".

I'm in favor of #14197 as it can catch possible errors, and it doesn't seem easy to write a selector that would disallow sequence expressions everywhere except in for-loop init/update.

@danielrentz
Copy link
Contributor Author

Sure it does not hide errors like #14197 but is mostly for completeness :)

@nzakas
Copy link
Member

nzakas commented Aug 28, 2021

We aren’t moving forward with this, so closing.

@nzakas nzakas closed this as completed Aug 28, 2021
Triage automation moved this from Evaluating to Complete Aug 28, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 25, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

No branches or pull requests

3 participants