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

parentheses around arrow function single argument #414

Closed
whitecolor opened this issue Feb 10, 2016 · 42 comments

Comments

@whitecolor
Copy link

commented Feb 10, 2016

Standard makes to wrap with parentheses single argument in simple cases:

[1, 2, 3].map((number) => `A string containing the ${number}.`)

and even the simpliest (which are very popular in functional/streams programming)

some.filter((x) => x)
// or
some.flatMap((_) => _)

this seems to me more readable and doesn't break consitency:

some.filter(x => x)
// or
some.flatMap(_ => _)

arbnb has the same request a while ago (airbnb/javascript#281) and they eventually changed it: https://github.com/airbnb/javascript#arrow-functions to accomodate simplier cases.

I think it would be reasonable to do.

@rstacruz

This comment has been minimized.

Copy link
Member

commented Feb 10, 2016

come to think of it i've started using _ => _ too.

https://github.com/rstacruz/pnpm/blob/master/lib/install.js#L95-L103

@rstacruz

This comment has been minimized.

Copy link
Member

commented Feb 10, 2016

If your function takes a single argument and doesn’t use braces, omit the parentheses.

Can eslint actually do this? (edit: no it can't)

@whitecolor

This comment has been minimized.

Copy link
Author

commented Feb 10, 2016

@nettofarah

This comment has been minimized.

Copy link

commented Feb 10, 2016

Yeah, I was curious about this rule too.
So I'm guessing there's not much we can do here?

@feross

This comment has been minimized.

Copy link
Member

commented Feb 10, 2016

This was an intentional rule addition in standard v6. See: #309

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

some.flatMap(_ => _)

this looks heaps confusing to me - can't imagine newcomers to the language having a good time trying to remember the 6 or so ways functions can be declared nowadays - the more consistent we can keep it, the better

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 11, 2016

@yoshuawuyts @feross I actually use x => x a lot.
Especially for things like array.filter(x => x)... it'd be really annoying on second thought to have to do array.filter((x) => x)...

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

@dcousens yeah, same here actually - it's way easier to type. Though when reading back, it can be kinda hard to distinguish between var assignments, function calls and equality checks. For us it might be ok, but imagine being tired / intoxicated or less experienced in the language of JavaScript - this wouldn't be as easy to deal with.

// this is not uncommon hey, braces make things clearer
var baz = array.filter(x => x <= y)
var baz = array.filter(x => x >= y % 5 ? y : 1)
// easier to figure out while squinting eyes (or tired)
var baz = array.filter((x) => x <= y)
var baz = array.filter((x) => x >= ((y % 5) ? y : 1))

Also don't wanna have to distinguish between all these, less forms = better

var foo = function () {}
function foo () {}
function * foo () {}
var foo = {
  bar () {
  }
}
foo.filter(x => x)
foo.filter((x) => x)
foo.filter(x => {
  return x
})
foo.filter((x) => {
  return x
})
async x => x
async (x) => x
async x => {
  return x
}
async (x) => {
  return x
}
@bjornharrtell

This comment has been minimized.

Copy link

commented Feb 12, 2016

I miss x => x. Almost as much as I don't miss ;.

@whitecolor

This comment has been minimized.

Copy link
Author

commented Feb 12, 2016

I think this should be reconsidered, And allow usage of simple things like:
.filter(x => x)
Probably this is ok to enforce parens (like it is done in airbnb):

(some) => {
  // go complex there
}

Consistency and constrains are a great things, but the general task is to find a right balance.

@bcomnes

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

I kinda feel like this was too early to call.

@benji6

This comment has been minimized.

Copy link

commented Feb 13, 2016

@bjornharrtell I think this is a really important point, I really like standard because it allows me to write code that is both concise and safe. Enforcing parentheses feels a lot like enforcing semicolons to me. This decision was a shock to me because if anything I would have expected Standard to enforce no parentheses in unary arrow functions. As with semicolons the syntax may improve readability to those with very little experience in the language, but ultimately it's an enforcement of pointless keystrokes. I feel the same principle behind Standard's stance on semicolons should be applied in the case of unary arrow functions or we are left with an inconsistent set of rules.

@whitecolor

This comment has been minimized.

Copy link
Author

commented Feb 13, 2016

I would also enforced parens when you define function if it is possible:

var sample = (items) => items[Math.floor(Math.random()*items.length)]

But when it is just used as an argument .filter(x => x) it is ok without parens.

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

Maybe we should just see if we can special case x => x haha.

@timoxley

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2016

Agreed, this was a bit surprising, I had a mild rant when I discovered this change.

I kinda feel like this was too early to call.

👍

This decision was a shock to me because if anything I would have expected Standard to enforce no parentheses in unary arrow functions.

👍

We might have missed the boat for adjusting this though, as standard changing something that's used so pervasively may be too disruptive/divisive.

Regardless, I've been playing around with formatting arrows a bit and just prior to the standard v6 release I'd almost settled on what I think is a nice balance between managing complexity, consistency RE function arity and preserving terseness:


Proposal

  • Never parens for single-line arrows
  • Always multi-line for multiple args
  • Always parens for multi-line, even for single arg
  • No blocks or unnecessary parens in single-line arrow

edit: Also in favour of simply disallowing no parens and multiline as per @dcousens comment below

Never parens for single-line arrows

// OK
items.map(item => item.price)
// NOT OK
items.map((item) => item.price)

Always multi-line for multiple args

Passing multiple args on a single line would not be valid:

// NOT OK
items.map((item, index) => item.price * index)

Instead this would necessarily be a multi-line structure, either as a block:

// OK
items.map((item, index) => {
  return item.price * index
})

or as a multi-line, paren-enclosed expression:

// OK
items.map((item, index) => (
  item.price * index
))

Always parens for multi-line, even for single arg

Parens needed anyway once you have multiple arguments:

// OK
items.map((item, index) => {
  return item.price * index
})

But for consistency, this proposal would also enforce parens around a single arg in any multi-line function:

// NOT OK
items.map(item => {
  return item.price
})
// OK
items.map((item) => {
  return item.price
})

This also reduces some paren-adding hassle when switching from single arg to multiple arg, which always felt kinda weird to me.

No blocks or unnecessary parens in single-line arrow

Single-line arrows with a block or unnecessary parens should be split to multi-line or single line without block:

// NOT OK
items.map(item => { return item.price })
// NOT OK
items.map((item) => ( item.price ))

The pattern outlined above keeps single line arrows terse, but also restricts use of single-line arrows to only the most simple of functions – this is where they excel. The value proposition of single-line arrows drops dramatically as the complexity of the function goes up. Once you're introducing blocks or multiple arguments, standard could enforce the spanning the code across multiple lines.

Not sure if there are currently appropriate rules for any of this in eslint.

Adapted from nodejs/node#4813 (comment)

@LinusU

This comment has been minimized.

Copy link
Member

commented Feb 14, 2016

Always multi-line for multiple args

👎

I think that this is very unnecessary, consider the following example

Array.from({length: 5}, (_, idx) => idx)

Is it really fair to force me to write this instead?

Array.from({length: 5}, (_, idx) => {
  return idx
})

You haven't addressed zero-arguments either, what do you think about them?

Array.from({length: 5}, () => 1)

vs.

Array.from({length: 5}, () => {
  return 1
})

And also, I'm agreeing with that always requiring parentheses is a bit too much. Especially when writing the identity function :)

Array.from(arguments).filter(x => x)
@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 14, 2016

@feross this one is harder than expected.
Infact, I'd wager, there is still no tried and tested "best practice" for this yet.

Except maybe the best first step is just in disallowing: no parens with multiline.
That is just horrible and should be shot.

@timoxley

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

You haven't addressed zero-arguments either, what do you think about them?

@LinusU Multiline optional, "No blocks or unnecessary parens in single-line arrow" applies.

I guess there's also a _ vs () discussion for no arg.

Is it really fair to force me to write this instead?

Note that only needing the index is a bit of an edge-case. Could special-case when proceeding arg is _, but this is far getting too magical and would require writing new eslint rules

Except maybe the best first step is just in disallowing: no parens and multiline.

This is vastly simpler than my proposal and probably offends the least amount of people. 👍 @dcousens

@yyx990803

This comment has been minimized.

Copy link

commented Feb 17, 2016

This is the only thing preventing me from using standard 6 as-is. I'm okay about enforcing parens for multiline arrow functions, but really, enforced parens for one-liners like x => x is just too much :/

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

@feross any chance we could relax this rule a little?

@whitecolor

This comment has been minimized.

Copy link
Author

commented Feb 18, 2016

Well kinda general notice, arrow functions gave us a great power to express with more cleaner code like never before, but now we are are imposing restrictions on with parentheses.

Parentheses are evil when they do mess with each other, need for parens for multiple arguments Promise((resolve, reject) => ... and wrapping returning object => ({a:1, b:}) I understand as nesessary evil, but why impose them if they are not really needed, when they only add a mess?

What is multi line? Just as example of live code, with multiline:

  getItems: (keys) =>
      Array.isArray(keys)
        ? Promise.all(keys.map(key =>
          store.getItem(key).then(value => ({key, value}))
        ))
        : store.keys().then(keys => actions.getItems(keys))

Wrapping with parens:

  getItems: (keys) =>
      Array.isArray(keys)
        ? Promise.all(keys.map((key) =>
          store.getItem(key).then((value) => ({key, value}))
        ))
        : store.keys().then((keys) => actions.getItems(keys))

does it make more clear/readable? I doubt it.

@timdp

This comment has been minimized.

Copy link

commented Feb 20, 2016

@whitecolor While I'm a big fan of ?:, personally, I consider that example way too convoluted for a single expression. Consequently, I think it illustrates quite well how adding brackets would make the code more readable if it got refactored a bit first. Imagine it with an if and one or two helper functions.

@timdp

This comment has been minimized.

Copy link

commented Feb 20, 2016

So I guess my take is: favor readability over brevity and add those brackets. It's not a big deal.

@timoxley

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2016

@timdp readability is subjective. my take is: favour readability over brevity and remove those brackets. It's not a big deal.

@timdp

This comment has been minimized.

Copy link

commented Feb 20, 2016

You're not favoring anything over brevity when you're removing code for the sake of removing it. But I do agree that there's a subjective component to it.

@75lb

This comment has been minimized.

Copy link

commented Feb 25, 2016

i'm against this rule.. the reduced verbosity of arrow functions is such a welcome addition, forcing pointless parens just stinks it up again.

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

I personally still haven't upgraded my projects to standard 6.0.0 simply because of how much this new rule broke everything.

@timdp

This comment has been minimized.

Copy link

commented Feb 26, 2016

It's easy to apply though: replace (\w+) => with ($1) => in any modern text editor. That's what I did.

@benji6

This comment has been minimized.

Copy link

commented Feb 26, 2016

@dcousens I've held off on upgrading to 6 too across all our projects just on the basis of this rule, and @timdp I agree it's simple to fix I just don't see any benefit. In my earlier comment I wanted to put forward the idea that this rule is inconsistent with the rest of standard and I'm going to elaborate on that now:

  • standard forces me to omit pointless semicolons, but forces me to include pointless parentheses around unary arrow functions
  • standard allows me to omit pointless curly parentheses around single line for, if and while blocks, but forces me to include pointless parentheses around unary arrow functions
  • standard allows me to omit pointless curly parentheses around arrow function bodies, but forces me to include pointless parentheses around unary arrow functions

I just don't get the reasoning, if it was to improve readability then from my subjective point of view and speaking on behalf of other developers I know this rule has failed, but hey, there will be arguments about this because it is totally subjective. What I loved about standard was that unlike other linters it didn't patronize me by assuming some features of the language were too confusing for me to understand or use, but I definitely feel massively patronized by this rule!

Who'd have thought I'd be so upset by those extra two characters haha! But yeah, it's kind of a deal breaker for me as far as standard is concerned, other people may love it though I guess.

@feross

This comment has been minimized.

Copy link
Member

commented Feb 28, 2016

This is probably going to change in the next release of standard. Stay tuned folks.

@whitecolor

This comment has been minimized.

Copy link
Author

commented Feb 28, 2016

I think parens restrictions for arrow function should not be imposed at all, even for multiline, because exessive parens definitly spoil cleaness of the code.

sotojuan added a commit to sotojuan/redux-ava that referenced this issue Feb 29, 2016

@chrisdickinson

This comment has been minimized.

Copy link

commented Mar 15, 2016

FWIW, I tend to omit parens on any single-argument arrow function passed as an arg to a function, since I'm mostly using them to hand off to promises:

// e.g.,
somePromiseReturningFn().then(value => {
})

In this case the "enforce parens on multi-line" wouldn't work for me.

@dcousens

This comment has been minimized.

Copy link
Member

commented Mar 15, 2016

Any update @feross? Myself and the team I work with have had to hold off upgrading to v6 because of this, all hoping it'll be loosened up 👍

@feross

This comment has been minimized.

Copy link
Member

commented Mar 16, 2016

@dcousens Want to send a PR to eslint-config-standard to get this kicked off?

@mmazzarolo

This comment has been minimized.

Copy link

commented Mar 29, 2016

Any info on the new release date?

@linde12

This comment has been minimized.

Copy link

commented Mar 29, 2016

Just saw this discussion and wanted to say that i wish parens would always be required. It would make changes like this more convenient:
arr.map(u => user.name)
-->
arr.map((u, idx) => { return u.name + u.idx; })

That's just my thought on the topic. Sorry if this comment looks weird, written on my phone!

@feross

This comment has been minimized.

Copy link
Member

commented Apr 24, 2016

standard version 7.0.0-beta.0 was just released with the arrow parens rule disabled. Give it a try:

npm install -g standard@7.0.0-beta.0

@feross feross closed this Apr 24, 2016

@whitecolor

This comment has been minimized.

Copy link
Author

commented Apr 24, 2016

@feross thanks for improvment, I think it would be useful to add beta dist-tag on the package

@feross

This comment has been minimized.

Copy link
Member

commented Apr 24, 2016

Done: npm install -g standard@beta

kvz added a commit to kvz/locutus that referenced this issue Apr 25, 2016

Upgrade node dependencies
- bump eslint
- Removed "arrow-parens" rule
  From: standard/standard#414
- Merge pull request #28 from mmazzarolo/patch-1
  Removed "arrow-parens" rule
- New rule: Disallow unnecessary escape usage (no-useless-escape)
- Disallow duplicate imports (no-duplicate-imports)
- Disallow unmodified conditions of loops (no-unmodified-loop-condition)
- Disallow whitespace before properties (no-whitespace-before-property)
- Require Camelcase (camelcase)
- 5.2.0
@dcousens

This comment has been minimized.

Copy link
Member

commented Nov 24, 2016

To sum up, I think? the only rule we wanted here was: must have parens for multi-line lambda (that isn't to say that rule exists though)

@dcousens dcousens added the question label Nov 24, 2016

@feross

This comment has been minimized.

Copy link
Member

commented Nov 24, 2016

@dcousens I think that's right.

@mightyiam

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2016

I've wrote some more thoughts regarding this in my PR, suggesting "arrow-parens": [2, "as-needed"]. If you care, please check it out.

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.