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-extra-parens and no-cond-assign ("except-parens") don't play well #3317

Closed
spadgos opened this Issue Aug 7, 2015 · 12 comments

Comments

Projects
None yet
6 participants
@spadgos
Copy link
Contributor

commented Aug 7, 2015

Maybe I'm missing something, but it seems to be impossible to use the no-extra-parens rule alongside no-cond-assign, with its exception for conditionals wrapped in parentheses (the default).

That is, there's no way (that I've found) to express this code without an error with both of the rules turned on:

while ((foo = bar())) { ... }

no-cond-assign by default requires assignments within conditionals to be wrapped, and errors if it isn't. no-extra-parens errors if you do use the parens.

Just as it has exceptions for regexes, I would suggest that no-extra-parens should not complain about using parens to wrap an assignment in a conditional.


Info for @eslintbot: Using eslint 1.0.0, with the code above, and no-cond-assign and no-extra-parens turned on.

@eslintbot

This comment has been minimized.

Copy link

commented Aug 7, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

I would not want this to be the default, but that sounds like a useful option.

@ilyavolodin ilyavolodin added the triage label Aug 7, 2015

@nzakas

This comment has been minimized.

Copy link
Member

commented Aug 11, 2015

That's an interesting conundrum. We probably need an option on no-extra-parens to allow that.

@nzakas nzakas added enhancement rule accepted and removed triage labels Aug 11, 2015

@alberto

This comment has been minimized.

Copy link
Member

commented Jan 9, 2016

Shouldn't be this the default behaviour? Adding the extra parens seems explicit enough to me. Anyway, if we were to add it, should it be a third option (for example all-except-cond-assign) or an additional argument for all?

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 9, 2016

Are you saying we should always allow parens when there's an assignment?

@alberto

This comment has been minimized.

Copy link
Member

commented Jan 9, 2016

Yes, allow double parens around the assignment inside conditionals, so these would be valid.

"while ((foo = bar())) {}",
"if ((foo = bar())) {}",
"do; while ((foo = bar()))",

Do you think that would be a problem?

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Jan 9, 2016

This needs to be behind an option though.

@alberto

This comment has been minimized.

Copy link
Member

commented Jan 9, 2016

@michaelficarra can you elaborate on why? Maybe I'm missing something

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 10, 2016

The only problem is that those are unnecessary parens, so we are kind of breaking the rule if we allow it by default. Adding as an option would let people opt-in.

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Jan 10, 2016

Exactly.

@alberto

This comment has been minimized.

Copy link
Member

commented Jan 10, 2016

I proposed that because that's what the rule already does with Regexps and IIFEs.

Is the name all-except-cond-assign ok for you?

alberto added a commit that referenced this issue Jan 10, 2016

alberto added a commit that referenced this issue Jan 10, 2016

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

Yeah, I really wish those were behind flags as well, but it's probably too late for that now.

alberto added a commit that referenced this issue Jan 11, 2016

@gyandeeps gyandeeps closed this in d487013 Jan 12, 2016

gyandeeps added a commit that referenced this issue Jan 12, 2016

Merge pull request #4911 from eslint/issue3317
Update: Option to allow extra parens for cond assign (fixes #3317)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.