tools: lint for function argument alignment

[Trott]

Checklist

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

Affected core subsystem(s)

tools benchmark lib test

Description of change

In function calls that span multiple lines, apply a custom lint rule to
enforce argument alignment.
With this rule, the following code will be flagged as an error by the
linter because the arguments on the second line start in a different
column than on the first line:
myFunction(a, b,
c, d);
The following code will not be flagged as an error by the linter:
myFunction(a, b,
c, d);

[Trott]

Not sure if folks will like this one or not, but this is the other thing I always get nits about (hi, @bnoordhuis!). So rather than keep messing it up every time the rest of my life and spending valuable time force pushing to fix it, maybe the linter can tell me when I've messed it up and no one has to know.

[jasnell]

I'm generally +1 on this. LGTM but let's here from others.

[Trott]

rebased, force pushed...

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

[Trott]

Only failure in CI is a known problem test that @cjihrig has a pending pull request that fixes it.

[Trott]

/bump @nodejs/collaborators

Any enthusiastic endorsements or horrified objections to this?

[targos]

the alignment seems wrong to me here

[benjamingr]

@tergos do you mean wrong as in you disagree with it personally?

[targos]

I'm not sure. It looks wrong to me because both strings are not aligned to each other

[Trott]

@targos Simplifying a bit, this is what the above code boils down to:

func1(arg1String,arg2Func(function(){returnfoo;},innerArgString));

I wonder if the real obstacle to readability is the string concatenation (which makes it look like two string arguments when it's just one).

Also, the final closing parenthesis might make more sense on its own line.

Would removing the concatenation and moving the parenthesis help? Or is the issue something else?

[Trott]

@targos@benjamingr I've rebased, rewritten these lines to hopefully conform more to expectations, and force pushed. Here's what they look like now:

// Legacy APIfunctiondefineLegacyGetter(legacyName,tlsName){functionwrapped(){returnrequire('tls')[tlsName];}constmsg=`crypto.${legacyName} is deprecated. Use tls.${tlsName} instead.`;exports.__defineGetter__(legacyName,internalUtil.deprecate(wrapped,msg));}defineLegacyGetter('createCredentials','createSecureContext');defineLegacyGetter('Credentials','SecureContext');

[targos]

I'm also +1 on the change. I think it improves readability.
@Trott did you try to propose this rule upstream ?

[claudiorodriguez]

+1 on this change - I remember seeing different styles all around, sticking to a single one and enforcing it with linting will definitely help readability while switching files.

[benjamingr]

+1 and generally LGTM.

[benjamingr]

Was moving the arrow a line down necessary here for the rule?

[Trott]

It wasn't the only option, but yes. Moving it meant that the next line didn't have to be indented so much that it exceeded the 80-character limit on line length.

[jasnell]

I'm generally not a fan of moving the first argument down if it can be avoided but can live with this.

[Trott]

@jasnell There are certainly other options, like assigning the function to a variable so you can just do assert.throws(foo,...).

[Trott]

By the way, the previous indentation is, in my opinion at least, unfriendly to the reader:

assert.throws(()=>{assert.doesNotThrow(makeBlock(thrower,Error),'user message');},/Gotunwantedexception.usermessage/,'a.doesNotThrow ignores user message');

I think moving the arrow function down one line is worth getting code that is easier to understand at a glance:

assert.throws(()=>{assert.doesNotThrow(makeBlock(thrower,Error),'user message');},/Gotunwantedexception.usermessage/,'a.doesNotThrow ignores user message');

[benjamingr]

This is going through a lot of code though, I wonder if we can't do this more incrementally.

[Trott]

@targos asked:

@Trott did you try to propose this rule upstream ?

Not yet, but that was the plan. There's a loophole in the implementation (for a common case with timers) that will probably be deemed larger than it needs to be. I suspect that if the upstream project wants the rule, they will want that tightened up a bit, which would be nice.

[Trott]

This is going through a lot of code though, I wonder if we can't do this more incrementally.

Do you mean multiple PRs? Or just split this PR into more commits? Or...

[Trott]

Rule proposed upstream: eslint/eslint#5946

[Trott]

I'm going to close this in favor of #6390 which is a more lenient version of this rule and therefore has a smaller (and hopefully uncontroversial) changeset.