Trailing comma rule now encompasses function decls, exprs and types

[markwongsk]

Fixes #1409

[palantirtech]

Thanks for your interest in palantir/tslint, @markwongsk! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[markwongsk]

The following currently do not work:

• Typed parameters in class declarations, interface declarations, function declarations
• Methods in classes

I am working on another commit to address those cases.

[adidahiya]

cool; what's the "methods in classes" case look like?

[jkillian]

Gave it a brief glance over, looking good so far 👍

[markwongsk]

@adidahiya methods in a class or interface declaration have node.kind of MethodDeclaration and SignatureDeclaration respectively, which aren't covered by the walker yet.

[markwongsk]

There is are a couple more issues that I have to address:

• ObjectType literals do not currently work
• Enums do not work

I also have a couple of test cases left. I've combed through the grammar but have no confidence on how thorough I've been. Please point out any remaining cases that you think we should support.

[markwongsk]

need to be removed

[markwongsk]

This is now ready for review.

[jkillian]

I'm gonna test this out on our codebase, but I don't anticipate you having to change much. Do you mind actually making this PR against master? Next release is going to be TSLint 4.0 which will only support >= TS 2.

And sorry for the slow turnarounds!

[jkillian]

I think this might be a bug:

const colon = node.getChildren().filter((child) =>
    child.kind === ts.SyntaxKind.ColonToken //warning for missing trailing comma here
)[0];

[jkillian]

Requests a comma here as well - thoughts?

export interface TestResult {
    directory: string;
    results: {
        [fileName: string]: {
            errorsFromLinter: LintError[];
            errorsFromMarkup: LintError[];
            fixesFromLinter: string;
            fixesFromMarkup: string;
            markupFromLinter: string;
            markupFromMarkup: string;
        } // missing comma
    };
}

[markwongsk]

For the first one, it's a bug. For the second one, this PR makes it such that a comma is indeed necessary there (it compiles with the comma) -- similar to how if it was an object, you would need a comma there.

[jkillian]

Ah yes, you're right. Just the first one needs to get fixed, second one is fine 👍

[markwongsk]

So I took a further look into the first example, and I think it is expected after this change. This is because the lambda happens to be an argument to a function, and multilined arguments to a function need to have a trailing comma. Consider

foo(
   arg1,
);

as the base behavior. It just so happens that in this case, arg1 is a lambda. Thus,

foo(a =>
    doSomethingWithA(a),
);

I think after this change,

foo(
    a => doSomethingWithA(a),
);

might be clearer. Thoughts?

[markwongsk]

I updated the base branch to master. Let me know if you require anything else :)

[jkillian]

Looks good, thanks @markwongsk!

[timocov]

Is there workaround to skip that check? I like trailing comma in multiline "objects", but function calls it is awful. IMHO