tools: enforce arrow function brace usage linting

[Trott]

Checklist

• tests and code linting passes
• the commit message follows commit guidelines

Affected core subsystem(s)

tools test lib

Description of change

Enable linting such that if braces are not required for an arrow function body, omit them.

Refs: #6390 (diff) (where there was a nit about this and I'd rather tools tell me code style nits rather than people...)

/cc @bnoordhuis

[jasnell]

Not sure about this one. I don't mind omitting the braces for simple, short expressions (I do so myself all the time) but they are quite helpful for readability if the expression is fairly complex.

[Trott]

@jasnell wrote:

Not sure about this one. I don't mind omitting the braces for simple, short expressions (I do so myself all the time) but they are quite helpful for readability if the expression is fairly complex.

True, although you can add outer parentheses in those cases and it probably is more readable than adding braces:

(foo) => (foo.bar().baz === ((bip << bap) / 4))

vs.

(foo) => { return foo.bar().baz === ((bip << bap) / 4); }

[jasnell]

Heh, I don't find the outer parens easier to read at all ;)

[Trott]

Heh, I don't find the outer parens easier to read at all ;)

Fair enough. The other approach, which you may find equally unconvincing, is that if you have a single expression that is so complicated that it needs to be put into a block explicitly for it to be readable, maybe it really shouldn't be a single expression. So:

// arguably bad
(foo) => { 
  return foo.bar().baz === ((bip << bap) / 4);
}

vs.

// arguably better
 (foo) => { 
  const blip = foo.bar().baz;
  const blap = (bip << bap) / 4;
  return blip === blap;
}

Obviously, some of this is aesthetics and reasonable people will have different opinions on what makes sense. If we can reach consensus here on a single consistent style/rule for arrow functions, then great. If we can't, oh well. ¯_(ツ)_/¯

[jasnell]

hmmm.. ;-) yeah, I'd rather not have to do that either. I'll stew on it to see if I can come up with some metric around what makes sense here.

[Trott]

@jasnell is the only person to offer an opinion on this so far and he doesn't support it. I'd like to leave this open for another 24-48 hours to give others a chance to offer other opinions. Will close for sure if there is no one who wants to endorse it after that.

As the risk of inviting everyone to an Ultimate Bikeshedding Party: /cc @nodejs/collaborators

[addaleax]

Nah, I’d be -1 on this too. Either style can be appropriate, depending on the situation, and sometimes it’s better left to humans to decide what’s more readable for other humans.

[mscdex]

In general I prefer the non-ES6 function syntax, so I would prefer to see braces (not parens) enforced as it is closer to non-ES6 function syntax.

[cjihrig]

I'm a fan of braces always.

[benjamingr]

-1, compare:

arr.filter(function(x) {
  return x > 2;
}).map(function(x) {
  return x * 3;
}).reduce(function(x,y) {
   return x + y;
});

With

arr.filter(x => x> 2).map(x => x*3).reduce((x,y) => x+y)

I find the latter a lot more readable and it's gaining a lot of momentum anyway.

[evanlucas]

I find the first a lot more readable. I'm +1 for always requiring parens and curly braces

[Trott]

@benjamingr The code you describe as more readable would pass this lint rule.

EDIT: Oh, I see, your -1 referred to someone else's comment/suggestion and not the PR.

[Trott]

I'm +1 for always requiring parens and curly braces

Unfortunately, always requiring braces would flag 160 problems in over 50 files in the current code base. The proposed rule here, in contrast, flags just 5 files.

[Trott]

As likely everyone suspected, there's a diversity of opinions and reaching consensus isn't going to happen anytime soon. That's about what I expected, but didn't want to assume.

I'm going to go ahead and close this. By all means, re-open if you want to champion this or if you think my assessment of the situation is wrong.