New Rule: newline-before-return

[artursvonda]

PR checklist

• Addresses an existing issue:
• New feature, bugfix, or enhancement
• Includes tests
• Documentation update

What changes did you make?

A new rule to force blank lines before return. Based on http://eslint.org/docs/rules/newline-before-return

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

Making sure all test cases are covered. I took the base file from eslint and added a few cases.

[new-rule] newline-before-return

[palantirtech]

Thanks for your interest in palantir/tslint, @artursvonda! 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.

[ajafff]

getChildren does not return what you expect, that's why indexOf does not work here:

node.parent is either ts.BlockLike or ts.IfStatement, ts.IterationStatement, ...
If it is not ts.BlockLike the return statement is the only child, e.g. an if statement without a block and only a single return statement. So you don't have to call getChildren.
If it is ts.BlockLike getChildren will return [OpenBraceToken, SyntaxList, CloseBraceToken] where SyntaxList is an array of statements within the block. But you don't need this SyntaxList, because BlockLike has a statements property (which also contains all statements, duh)

here is a simpler solution:

private getIndex(node: ts.Node) {
  const parent = node.parent!; // use ! (non null assertion) because we know every node has a parent (except ts.SourceFile)
  if (isBlockLike(parent) {
    return parent.statements.indexOf(node);
  } {
    // `node` is the only statement within this "block scope" -> do whatever you need to do in this case
  }
}
...
function isBlockLike(node: ts.Node): node is ts.BlockLike {
    return "statements" in node; // could also be node.kind === ts.SyntaxKind.Block || node.kind === ts.SyntaxKind.CaseClause || to be continued ...
}

[ajafff]

you could also just use all.search(/\S/) !== -1

[ajafff]

use this.getSourceFile() instead of node.getSourceFile()
the latter walks up the whole AST until it finds the SourceFile, which is unnecessary work

[artursvonda]

Thanks @ajafff for all the comments and suggestions. I addressed those comments and simplified the code a bit.

[nchen63]

what would happen there was a return on the first line?

does this handle other cases where someone writes a return statement on the same line as another statement?

[artursvonda]

Good point. Not sure how that should be handled. Any ideas/suggestions? Meanwhile I'll add a test case.

[nchen63]

I'm leaning toward always flagging if there's another statement on the same line as the return statement

[artursvonda]

@nchen63 Yeah, makes sense. The idea is that return should have spacing around it so it's clear and easier to spot and read. So the rule basically is "if the return is not the first thing in the block, there should be blank line before it.". With that in mind I simplified the code a bit more. Also changed the position of the error a bit and now it's at the start of the return statement.

[nchen63]

@artursvonda thanks!

[nchen63]

@artursvonda this rule is a good candidate for an auto fixer too

[artursvonda]

@nchen63 No problem. And yeah, will see if I can add the fixed over some evening.

[glen-84]

This should probably have been called blank-line-before-return.