Improve typedef for callback used as parameter by VincentLanglet · Pull Request #4533 · palantir/tslint

VincentLanglet · 2019-02-23T15:04:50Z

PR checklist

Addresses an existing issue: Inconsistency of typedef:parameter with lambdas #227 ; arrow-call-signature Not working with arrow function missing type definition #2000
Includes tests

Overview of change:

Typedef arrow-call-signature now return error for

someFunc(() => {});

Is there anything you'd like reviewers to focus on?

Actually, with all typedef rules actives.

const A = () => {} // error (return type)
someFunc(() => {}); // no error
someFunc((n) => {}); // error (param type)
setTimeout(function() {}, 1000); // error (return type)

After the PR, error everywhere ; for consistency.

Here #2000 (comment), a suggestion is a child-arrow-call-signature. That's a possibility, but we have to (for consistency) add a child-arrow-parameter too, which is also a suggestion here #813.
We, then, have to decide before if we want also a child-parameter and a child-call-signature.

I can do it if you prefer.
Personally I don't think all these options are necessary. The typedef is a style rule for people who want to type the code (more like a documentation) even if it's not necessary thanks to inference. If you don't want to be redundant and infer as much type you can, just use the noImplicitAny option in tsconfig.

CHANGELOG.md entry:

[enhancement] typedef rule arrow-call-signature option is more consistent in reporting errors on lambdas and will flag more violations that were missed in the previous rule implementation.

JoshuaKGoldberg

The code looks good to me (nice clean implementation!), but since this is a breaking change, I think we'd need to wait until 6.0 to merge in.

/cc @adidahiya

VincentLanglet · 2019-02-24T18:31:21Z

@JoshuaKGoldberg
The question "Is it breaking change or is it bug fix" is in a way kinda debatable.

There was no consistency in this example

const A = () => {} // error (return type)
someFunc(() => {}); // no error
someFunc((n) => {}); // error (param type)
setTimeout(function() {}, 1000); // error (return type)

So you can think it was a bug to not detect the error for callback return type.

VincentLanglet · 2019-03-04T16:36:46Z

@JoshuaKGoldberg @adidahiya Is it a breaking change or a bug fix ? I think it's debatable.

When is the v6 supposed to be released ?
If it's in a long time, an option can do the work while waiting

JoshuaKGoldberg · 2019-03-07T02:18:09Z

breaking change or a bug fix

Both? 😄

Even if this is considered a bug, it's a pretty big change. If a project already enables the rule under the expectation that scenario of arrow lambdas passed to methods shouldn't be flagged, then it starts flagging them, it'd become more difficult to migrate to the next TSLint version (even if this is desirable).

adidahiya

yes this is a breaking change and if you want to merge this it needs to be under a new option. I suggest naming the option check-call-signatures

adidahiya · 2019-03-12T05:26:13Z

 setTimeout(function() {}, 1000);

+const foo = n => n+1
+          ~~~~       [expected arrow-call-signature to have a typedef]


why does the failure range extend to the =? that seems a little off

In const foo = n => n+1 location.pos = 11
in const foo = (n => n+1) location.pos = 13

I didn't touch this part

adidahiya · 2019-03-12T05:26:23Z

+          ~~~~       [expected arrow-call-signature to have a typedef]
+
 someFunc(n => n+1);
+        ~~~       [expected arrow-call-signature to have a typedef]


same here, feels strange to include the ( in the failure range

It's made for (n) => n+1
.....................~~~ [expected arrow-call-signature to have a typedef]

The Node is only n.
But the code use this.addFailure(location.pos - 1, location.end + 1, failure);
I didn't touch this part

VincentLanglet · 2019-03-12T15:10:11Z

@JoshuaKGoldberg @adidahiya I agree it will be easier with something like arrow-callback-call-signature option. But it does not make any sense having an option for callback in arrow function, and not in standard function, or an option for callback return type in arrow function, and not parameter type in callback function.

A bug fix is always a breaking change. Before the rule had false positive, now it correctly find the errors.

But if you prefer, you can wait for the next major version. But in this case I have a question. When is it planned ? There is already a lot of Breaking changes waiting for it.
https://github.com/palantir/tslint/pulls?q=is%3Aopen+is%3Apr+label%3A%22Type%3A+Breaking+Change%22

JoshuaKGoldberg · 2019-07-27T14:57:12Z

@VincentLanglet great question! 😄 just filed #4811 to discuss.

VincentLanglet · 2019-08-04T19:21:27Z

@JoshuaKGoldberg Great ! Is this PR ready for 6.0.0 version ?

JoshuaKGoldberg · 2019-10-02T07:38:54Z

Deferring to @adidahiya for review now that we're ready for breaking changes (#4811).

adidahiya · 2019-10-05T19:17:39Z

thanks for the PR @VincentLanglet!

Improve typedef

3ffed49

JoshuaKGoldberg added PR: waiting for reviewer Type: Breaking Change labels Feb 24, 2019

JoshuaKGoldberg approved these changes Feb 24, 2019

View reviewed changes

adidahiya self-assigned this Feb 25, 2019

adidahiya suggested changes Mar 12, 2019

View reviewed changes

adidahiya removed the PR: waiting for reviewer label Mar 12, 2019

JoshuaKGoldberg added PR: waiting for reviewer and removed Type: Breaking Change labels Oct 2, 2019

adidahiya added Type: Breaking Change and removed PR: waiting for reviewer labels Oct 5, 2019

adidahiya approved these changes Oct 5, 2019

View reviewed changes

adidahiya merged commit 8356b58 into palantir:master Oct 5, 2019

JoshuaKGoldberg mentioned this pull request Nov 3, 2019

arrow-call-signature Not working with arrow function missing type definition #2000

Closed

adidahiya mentioned this pull request Nov 3, 2019

Prepare release v6.0.0-beta0 #4885

Merged

snebjorn mentioned this pull request Mar 25, 2020

tslint 6 compatibility mgechev/codelyzer#959

Closed

This was referenced Mar 14, 2021

Bump tslint from 5.20.1 to 6.1.3 jscheffner/vscode-workspace-terminals#7

Merged

Bump tslint from 5.20.1 to 6.1.3 in /app MicroPad/MicroPad-Core#293

Closed

Bump tslint from 5.17.0 to 6.1.3 Kerron-Hutton/lambda-manager#16

Open

Uh oh!

Conversation

VincentLanglet commented Feb 23, 2019 • edited by adidahiya Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

VincentLanglet commented Feb 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VincentLanglet commented Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshuaKGoldberg commented Mar 7, 2019

Uh oh!

adidahiya left a comment

Choose a reason for hiding this comment

Uh oh!

adidahiya Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

adidahiya Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

VincentLanglet commented Mar 12, 2019

Uh oh!

JoshuaKGoldberg commented Jul 27, 2019

Uh oh!

VincentLanglet commented Aug 4, 2019

Uh oh!

JoshuaKGoldberg commented Oct 2, 2019

Uh oh!

adidahiya commented Oct 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

VincentLanglet commented Feb 23, 2019 •

edited by adidahiya

Loading

VincentLanglet commented Feb 24, 2019 •

edited

Loading

VincentLanglet commented Mar 4, 2019 •

edited

Loading