Add arrow function rules to eslint

[cjihrig]

This PR enforces the following linting rules:

• arrow-parens - requires parens around arrow function arguments
• arrow-spacing - ensures a space on each side of the =>
• no-arrow-condition - prevents accidental use of => in cases
  where the user really intends to use <=

The only code that violated these rules existed in the test directory.
R= @rvagg

[rvagg]

lgtm

[Qard]

LGTM

[jasnell]

LGTM

[thefourtheye]

LGTM

[timoxley]

@rvagg

I find the terseness without them make them harder to visually parse when reading code.

This is truly only temporary from being used to ES5, it does go away but…

I've been thinking about the issue of arrow function parens a bit recently and I think there's a good case for a middle ground.

Perhaps consider this instead:

• No parens for single line arrows
• Always parens for multi-line, even if single arg
• Always multi-line for multiple args

Having more than one argument messes up terseness/simplicity of single line arrows anyway.
This keeps single line arrows nice and clean, but also restricts the use of single line arrow to simpler, one arg types of functions, which I think is where they excel. Value proposition of single line drops dramatically as complexity of the function goes up.

Also found you're far more likely to need to add multiple args for something that's already a multi-line function, so this pre-empts that and removes annoying need to add parens around just because you're adding one argument.

So trivial things like this remain nice and terse:

items.map(item => item.price)

but adding a second arg messes the terseness of a single line up even before using the arg in a function body:

items.map((item, index) => item.price)

Instead, the proposed option would force a multi-line structure for multi-args, like so:

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

Not sure if there's a rule for this in eslint.

[saghul]

LGTM.

[targos]

lgtm

[mcollina]

LGTM, but I'm also ok with @timoxley proposal.

[silverwind]

nit: no spaces inside braces.

[targos]

and technically the quotes are optional but we don't have consistent use of them in this file...

[silverwind]

Interesting, I wasn't aware that quotes are optional in most cases YAML. I'll investigate if we can remove all quotes.

[evanlucas]

LGTM. Definitely agree on this

[bnoordhuis]

So, am I the only one who finds parens around a single argument arrow function ugly?

[zeusdeux]

Nope. I am with you on that @bnoordhuis.

[Fishrock123]

Better to have less options here for the sake of new people who will inevitably become confused by the like 5 different cases of style for arrow functions. LGTM

[Qard]

Personally, I prefer to only use arrow functions in the single line, paren free style, and with short argument names.

The needing to explicitly return in multi line arrow functions but not single line ones is super confusing to new people, so I'd avoid that style entirely.

[Fishrock123]

Personally, I prefer to only use arrow functions in the single line, paren free style, and with short argument names.

Sure, but chances are we'll care more about the speed of the context capturing vs bind() over style.

[cjihrig]

CI: https://ci.nodejs.org/job/node-test-pull-request/1353/

[jasnell]

CI failures look unrelated. There are more than enough signoffs to land but given the discussion it may be worth while holding this one open for another day.

[jasnell]

@bnoordhuis ... I personally find arrow functions ugly in general along with the parens, but I'd rather we had consistency

[jasnell]

going to mark this one as do not land in v4 tho... can revisit that if folks feel strongly about it tho

[thefourtheye]

@jasnell If we do not backport linter rules, landing code which follow this would be difficult in the future, no?

[jasnell]

yes, if those rules are less restrictive or change an existing rule. In this case, not adding this rule shouldn't impact anything (tho I could be wrong)

[thefourtheye]

not adding this rule shouldn't impact anything

Yes, but I am thinking of a very rare case. Let's say we land a fix which is specific to 4.x and if that doesn't follow this rule, then we will have two slightly different style systems and it wouldn't be consistent, right?

[jasnell]

Yeah, you're right.
On Jan 22, 2016 10:04 PM, "thefourtheye" notifications@github.com wrote:

not adding this rule shouldn't impact anything

Yes, but I am thinking of a very rare case. Let's say we land a fix which
is specific to 4.x and if that doesn't follow this rule, then we will have
two slightly different style systems and it wouldn't be consistent, right?

—
Reply to this email directly or view it on GitHub
#4813 (comment).

[silverwind]

There's no Arrow Functions category in http://eslint.org/docs/rules/. Can you structure the rules like in that list?

[benjamingr]

I'm with @bnoordhuis writing e => e.text is nicer than (e) => e.text. The language lets us omit these because it is a simple one parameter case.

As a data point - linters for C# (like ReSharper's rules) raise a warning (by default) if you include the ()s in a single parameter. If I remember correctly (please correct me) so do Java IDEs like IntelliJ (for ->) and Scala.

[cjihrig]

@jasnell thoughts on landing this now? The nice thing about disabling or loosening a lint rule in the future is that it doesn't require going back and fixing the code again.