Tslint: newline-per-chained-call is too strict by default

[JoshuaKGoldberg]

Bug Report

• __TSLint version__: 5.9.0
• __TypeScript version__: 2.7.0-dev.20171126
• __Running TSLint via__: (pick one) CLI /& VSCode

TypeScript code being linted

const mySpy = jasmine.createSpy()
    .and.returnValue(true);
     // ~~~~~~~~~~~~ [tslint] When chaining calls, put method calls on new lines. (newline-per-chained-call)

// ...

expect(mySpy).toHaveBeenCalledTimes(1);
          // ~~~~~~~~~~~~~~~~~~~~~~ [tslint] When chaining calls, put method calls on new lines. (newline-per-chained-call)

with tslint.json configuration:

{
    "rules": {
        "newline-per-chained-call": true
    }
}

Actual behavior

Errors thrown for BDD-style syntax.

Expected behavior

BDD-style test libraries such as Chai and Jasmine look best when used on the same line. They should be allowed to do so.

Perhaps a couple of configurations:

• Allow non-method members (like .and) as opt-out
• Allow a maximum line length as opt-in

[aervin]

To me, this is a bug

const mySpy = jasmine.createSpy()
    .and.returnValue(true);
     // ~~~~~~~~~~~~ [tslint] When chaining calls, put method calls on new lines. (newline-per-chained-call)

because the offending call is the only call on the line. Currently, this throws for element access expressions too. I have a local fix and will open a PR.

Perhaps config updates should be merged separately? I like the max line-length idea.

[aervin]

Actually, I'll hold off on the PR as people may be interpreting the rule name differently than I am. Should chained calls be on a dedicated line?

[JoshuaKGoldberg]

IMO yes by default; I can't think of any situation in which they'd want to be.

Also, perhaps a configuration setting for a minimum number of calls to make it necessary to split lines?

Usage with Inversify:

const container = new Container();

container.bind(Logger).toSelf(); // should this be flagged? IMO no

[aervin]

Hmm, I still think this should be left alone

const mySpy = jasmine.createSpy()
    .and.returnValue(true); /* I'm not on the same line as the previous "link" */

but you're right, flagging this isn't great

container.bind(Logger).toSelf(); 

Something like this sounds good to me

"newline-per-chained-call": [true, {
    "max-calls-per-line": 2
}]

(I don't think we should bother supporting this configuration)

"newline-per-chained-call": [true, 2]

[JoshuaKGoldberg]

Oh for clarity: the .and.returnValue is what's giving an error.

[aervin]

@JoshuaKGoldberg I think .and.returnValue should not return an error, but we should add a config option to address this

container.bind(Logger).toSelf(); /* Allow n (2) calls per line */

[digable1]

+1 regarding this rule detracting from the readability of unit tests, while enhancing other usages such as with Observables.

Furthermore, will restricting the maximum number of chained calls to a line really help? For example:

doSomethingThatReturnsAnObservable(...).subscribe(() => {...})

would still be permissible if you're just using a counter in the configuration.

Are we really suggesting a set of allowed syntax that would be permissible on the same line in a chaining construct (regex allowed in the specification)?

[aervin]

To me, this rule solves one problem really well. Should we complicate it with a regex whitelist (concerns about scalability), or advise that people use specific tslint configs for .spec files, for instance?

[heavybeard]

Maybe you know, but I want to be sure: ES-Lint has the same rule with options.

I think should be used the same syntax for options

https://eslint.org/docs/rules/newline-per-chained-call

[wh1t3cAt1k]

+1 encountered today and had to turn the rule off. It is definitely very useful, but I would like to be able to partially opt out with the minimum number of chained calls to report error. As @heavybeard suggests, it would make sense to at least adopt that option from eslint.

[Mukhametvaleev]

I think consistent value would be good too:

{
    "rules": {
        "newline-per-chained-call": [true, "consistent"]
    }
}

[JoshuaKGoldberg]

💀 _It's time!_ 💀

TSLint is being deprecated and no longer accepting pull requests for major new changes or features. See #4534. 😱

If you'd like to see this change implemented, you have two choices:

• Recommended: Check if this is available in ESLint + typescript-eslint ✅
• _Not Recommended: Fork TSLint locally_ 🤷‍♂️

👋 It was a pleasure open sourcing with you!

_If you believe this message was posted here in error, please comment so we can re-open the issue!_

[JoshuaKGoldberg]

🤖 Beep boop! 👉 TSLint is deprecated 👈 _(#4534)_ and you should switch to typescript-eslint! 🤖

🔒 This issue is being locked to prevent further unnecessary discussions. Thank you! 👋