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

Should no-wrap-func allow this exception? #2402

Closed
mightyiam opened this issue Apr 28, 2015 · 12 comments
Closed

Should no-wrap-func allow this exception? #2402

mightyiam opened this issue Apr 28, 2015 · 12 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@mightyiam
Copy link

I have some testing code that goes like

describe('thing', function () {
  it('throws', function () {
    (function () {thing()}).should.throw
  }
}) 

The no-wrap-func trips on that. But the parentheses are that for a good reason—otherwise it would have been a function statement without a name—which is an error.

Is there a neater way to write this?

Should no-wrap-func recognize this pattern?

@bathos
Copy link

bathos commented Apr 28, 2015

A neater way might be to assign the function first, then use the reference -- depending on your preference. If I understand right, no-wrap-func is for those who feel parentheses as a means to expression-ize a function is bad for readability, with exception made for one very common pattern. So it looks like it's working right to me, unless no-wrap-func is actually supposed to just disallow expressionization that's not "used."

@ilyavolodin ilyavolodin added the triage An ESLint team member will look at this issue soon label Apr 28, 2015
@mightyiam
Copy link
Author

@bathos the "one very common pattern" would be

(function () {})()

where it is immediately called, and thus an IIFE?

If the former, then I'll write my code different.

Does anyone think that it is the latter? Well, it obviously isn't the latter.

Another rule for that could be no-unused-wrap-func.

@nzakas nzakas added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Apr 28, 2015
@nzakas
Copy link
Member

nzakas commented Apr 28, 2015

Yeah, this should be an exception.

@mightyiam
Copy link
Author

Thank you @bathos and @nzakas.

@michaelficarra
Copy link
Member

Why should this be an exception? If you don't want to wrap function expressions in unnecessary parentheses, prefix it with !.

describe('thing', function () {
  it('throws', function () {
    !function () {thing()}.should.throw
  }
}) 

@bathos
Copy link

bathos commented Apr 28, 2015

@mightyiam yes, IIFE is what I meant. I had assumed this rule exception existed only because of its frequency and peculiar nature (i.e., you would not want to assign an IIFE if the whole point of it is to avoid outer scope pollution). But it seems that assumption was not correct -- based on nzakas response it seems the rule was only meant to disallow parenthetical function expressions when the parentheses aren't serving a purpose, whatever it may be.

@michaelficarra Since it seems the purpose of the rule is only to avoid pointless expressionization, I think the fact that there are other ways to trigger evaluation as an expression isn't significant. I (subjectively, I'll emphasize) consider using the negation operator here to be misleading; parens say "I contain an expression," which is exactly what we want to communicate, while ! says "I am the boolean inverse of the truthiness of the following expression" -- and then we do nothing with the value we've created, since we didn't really "mean" it.

@michaelficarra
Copy link
Member

@bathos: You're already using an expression for its side effects. Seeing a ! or void in leading statement position indicates that to me more clearly. But of course, it is subjective as you say.

Why don't you turn off the rule? Is this not exactly the kind of thing this rule is meant to detect?

@bathos
Copy link

bathos commented Apr 28, 2015

That's fair, @michaelficarra. Everyone has their own take on what conveys intention clearly in code. You mentioned void -- that's actually the perfect choice in my eyes. I'd never thought of it before, I guess because it's over in the heap of unloved toys we're not supposed to want to touch :)

I'm not the OP, btw -- not sure if that last line was meant for me; I actually don't have this rule on, at least not on my current project. But the docs at http://eslint.org/docs/rules/no-wrap-func do make it sound like this is "exactly the kind of thing this rule is meant to detect," as you said. So if the OP's example becomes valid with no-wrap-func, I think the docs will need to change too.

@michaelficarra
Copy link
Member

Sorry @bathos, I meant that second paragraph for @mightyiam.

@nzakas
Copy link
Member

nzakas commented Apr 28, 2015

The purpose of this rule is to say "removing the parentheses doesn't change the meaning". However, in this case, removing the parentheses definitely does change the meaning unless you add something like void or !. So, I still feel like this is a valid exception to this rule.

@nzakas
Copy link
Member

nzakas commented May 8, 2015

Working on this.

@nzakas nzakas closed this as completed in c11c408 May 8, 2015
nzakas added a commit that referenced this issue May 8, 2015
Fix: Don't warn for member expression functions (fixes #2402)
@mightyiam
Copy link
Author

Thank you @nzakas and everyone on the discussion.

@nzakas I hope you figure out the rest of the issues from c11c408.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 7, 2018
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 archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

5 participants