Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update: add fixer for newline-before-return (fixes #5958) #7050

Merged
merged 4 commits into from Sep 7, 2016
Merged

Update: add fixer for newline-before-return (fixes #5958) #7050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
52b6ab0
Update: add fixer for newline-before-return (fixes #5958)
vitorbal Sep 3, 2016
0c352e2
Update: improve canFix() logic and add more tests (refs #5958)
vitorbal Sep 4, 2016
a68f4a1
Update: make fixer a bit smarter and add more tests (refs #5958)
vitorbal Sep 6, 2016
0b6e24b
Update: update docs and fix coding style issue (refs #5958)
vitorbal Sep 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/rules/newline-before-return.md
View file
Open in desktop
@@ -1,5 +1,7 @@
# require an empty line before `return` statements (newline-before-return)

(fixable) The `--fix` option on the [command line](../user-guide/command-line-interface#fix) automatically fixes problems reported by this rule.

There is no hard and fast rule about whether empty lines should precede `return` statements in JavaScript. However, clearly delineating where a function is returning can greatly increase the readability and clarity of the code. For example:

```js
Expand Down
71 changes: 63 additions & 8 deletions lib/rules/newline-before-return.js
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@ module.exports = {
category: "Stylistic Issues",
recommended: false
},

fixable: "whitespace",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be "code" instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be "whitespace" since we're only altering whitespace

schema: []
},

Expand Down Expand Up @@ -103,14 +103,13 @@ module.exports = {
}

/**
* Checks whether node is preceded by a newline
* @param {ASTNode} node - node to check
* @returns {boolean} Whether or not the node is preceded by a newline
* Returns the line number of the token before the node that is passed in as an argument
* @param {ASTNode} node - The node to use as the start of the calculation
* @returns {number} Line number of the token before `node`
* @private
*/
function hasNewlineBefore(node) {
const tokenBefore = sourceCode.getTokenBefore(node),
lineNumNode = node.loc.start.line;
function getLineNumberOfTokenBefore(node) {
const tokenBefore = sourceCode.getTokenBefore(node);
let lineNumTokenBefore;

/**
Expand All @@ -127,11 +126,58 @@ module.exports = {
lineNumTokenBefore = 0; // global return at beginning of script
}

return lineNumTokenBefore;
}

/**
* Checks whether node is preceded by a newline
* @param {ASTNode} node - node to check
* @returns {boolean} Whether or not the node is preceded by a newline
* @private
*/
function hasNewlineBefore(node) {
const lineNumNode = node.loc.start.line;
const lineNumTokenBefore = getLineNumberOfTokenBefore(node);
const commentLines = calcCommentLines(node, lineNumTokenBefore);

return (lineNumNode - lineNumTokenBefore - commentLines) > 1;
}

/**
* Checks whether it is safe to apply a fix to a given return statement.
*
* The fix is not considered safe if the given return statement has leading comments,
* as we cannot safely determine if the newline should be added before or after the comments.
* For more information, see: https://github.com/eslint/eslint/issues/5958#issuecomment-222767211
*
* @param {ASTNode} node - The return statement node to check.
* @returns {boolean} `true` if it can fix the node.
* @private
*/
function canFix(node) {
const leadingComments = sourceCode.getComments(node).leading;
const lastLeadingComment = leadingComments[leadingComments.length - 1];
const tokenBefore = sourceCode.getTokenBefore(node);

if (leadingComments.length === 0) {
return true;
}

// if the last leading comment ends in the same line as the previous token and
// does not share a line with the `return` node, we can consider it safe to fix.
// Example:
// function a() {
// var b; //comment
// return;
// }
if (lastLeadingComment.loc.end.line === tokenBefore.loc.end.line &&
lastLeadingComment.loc.end.line !== node.loc.start.line) {
return true;
}

return false;
}

//--------------------------------------------------------------------------
// Public
//--------------------------------------------------------------------------
Expand All @@ -141,7 +187,16 @@ module.exports = {
if (!isFirstNode(node) && !hasNewlineBefore(node)) {
context.report({
node,
message: "Expected newline before return statement."
message: "Expected newline before return statement.",
fix(fixer) {
if (canFix(node)) {
const tokenBefore = sourceCode.getTokenBefore(node);
const newlines = node.loc.start.line === tokenBefore.loc.end.line ? "\n\n" : "\n";

return fixer.insertTextBefore(node, newlines);
}
return null;
}
});
}
}
Expand Down
134 changes: 113 additions & 21 deletions tests/lib/rules/newline-before-return.js
View file
Open in desktop
Expand Up @@ -110,79 +110,171 @@ ruleTester.run("newline-before-return", rule, {
],

invalid: [
{
code: "function a() {\nvar b; return;\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b; \n\nreturn;\n}"
},
{
code: "function a() {\nvar b;\nreturn;\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b;\n\nreturn;\n}"
},
{
code: "function a() {\nif (b) return b;\nelse if (c) return c;\nelse {\ne();\nreturn d;\n}\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nif (b) return b;\nelse if (c) return c;\nelse {\ne();\n\nreturn d;\n}\n}"
},
{
code: "function a() {\nif (b) return b;\nelse if (c) return c;\nelse {\ne(); return d;\n}\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nif (b) return b;\nelse if (c) return c;\nelse {\ne(); \n\nreturn d;\n}\n}"
},
{
code: "function a() {\n while (b) {\nc();\nreturn;\n}\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\n while (b) {\nc();\n\nreturn;\n}\n}"
},
{
code: "function a() {\ndo {\nc();\nreturn;\n} while (b);\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\ndo {\nc();\n\nreturn;\n} while (b);\n}"
},
{
code: "function a() {\nfor (var b; b < c; b++) {\nc();\nreturn;\n}\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nfor (var b; b < c; b++) {\nc();\n\nreturn;\n}\n}"
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was a duplicate of the one before it, so I removed it.

{
code: "function a() {\nfor (b in c) {\nd();\nreturn;\n}\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nfor (b in c) {\nd();\n\nreturn;\n}\n}"
},
{
code: "function a() {\nfor (b of c) {\nd();\nreturn;\n}\n}",
parserOptions: { ecmaVersion: 6 },
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nfor (b of c) {\nd();\n\nreturn;\n}\n}"
},
{
code: "function a() {\nif (b) {\nc();\n}\n//comment\nreturn b;\n}",
errors: ["Expected newline before return statement."]
},
{
code: "function a() {\nif (b) {\nc();\n}\n//comment\nreturn b;\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nif (b) {\nc();\n}\n//comment\nreturn b;\n}"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some tests (like this one), I've added an output even though nothing changed, to guarantee that a fix is not applied in this case. Hope that's the correct approach!

},
{
code: "function a() {\n/*comment\ncomment*/\nif (b) {\nc();\nreturn b;\n} else {\n//comment\n\nreturn d;\n}\n/*multi-line\ncomment*/\nreturn e;\n}",
errors: ["Expected newline before return statement.", "Expected newline before return statement."]
errors: ["Expected newline before return statement.", "Expected newline before return statement."],
output: "function a() {\n/*comment\ncomment*/\nif (b) {\nc();\n\nreturn b;\n} else {\n//comment\n\nreturn d;\n}\n/*multi-line\ncomment*/\nreturn e;\n}"
},
{
code: "function a() {\nif (b) { return; } //comment\nreturn c;\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nif (b) { return; } //comment\n\nreturn c;\n}"
},
{
code: "function a() {\nif (b) { return; } /*multi-line\ncomment*/\nreturn c;\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nif (b) { return; } /*multi-line\ncomment*/\nreturn c;\n}"
},
{
code: "function a() {\nif (b) { return; }\n/*multi-line\ncomment*/ return c;\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nif (b) { return; }\n/*multi-line\ncomment*/ return c;\n}"
},
{
code: "function a() {\nif (b) { return; } /*multi-line\ncomment*/ return c;\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nif (b) { return; } /*multi-line\ncomment*/ return c;\n}"
},
{
code: "var a;\nreturn;",
parserOptions: { ecmaFeatures: { globalReturn: true } },
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "var a;\n\nreturn;"
},
{
code: "var a; return;",
parserOptions: { ecmaFeatures: { globalReturn: true } },
errors: ["Expected newline before return statement."],
output: "var a; \n\nreturn;"
},
{
code: "function a() {\n{\n//comment\n}\nreturn\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\n{\n//comment\n}\n\nreturn\n}"
},
{
code: "function a() {\n{\n//comment\n} return\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\n{\n//comment\n} \n\nreturn\n}"
},
{
code: "function a() {\nvar c;\nwhile (b) {\n c = d; //comment\n}\nreturn c;\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nvar c;\nwhile (b) {\n c = d; //comment\n}\n\nreturn c;\n}"
},
{
code: "function a() {\nfor (var b; b < c; b++) {\nif (d) {\nbreak; //comment\n}\nreturn;\n}\n}",
errors: ["Expected newline before return statement."]
errors: ["Expected newline before return statement."],
output: "function a() {\nfor (var b; b < c; b++) {\nif (d) {\nbreak; //comment\n}\n\nreturn;\n}\n}"
},

// Testing edge cases of the fixer when the `return` statement has leading comments.
// https://github.com/eslint/eslint/issues/5958
{
code: "function a() {\nvar b; /*multi-line\ncomment*/\nreturn c;\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b; /*multi-line\ncomment*/\nreturn c;\n}"
},
{
code: "function a() {\nvar b;\n/*multi-line\ncomment*/ return c;\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b;\n/*multi-line\ncomment*/ return c;\n}"
},
{
code: "function a() {\nvar b; /*multi-line\ncomment*/ return c;\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b; /*multi-line\ncomment*/ return c;\n}"
},
{
code: "function a() {\nvar b;\n//comment\nreturn;\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b;\n//comment\nreturn;\n}"
},
{
code: "function a() {\nvar b; //comment\nreturn;\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b; //comment\n\nreturn;\n}"
},
{
code: "function a() {\nvar b;\n/* comment */ return;\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b;\n/* comment */ return;\n}"
},
{
code: "function a() {\nvar b;\n//comment\n/* comment */ return;\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b;\n//comment\n/* comment */ return;\n}"
},
{
code: "function a() {\nvar b; /* comment */ return;\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b; /* comment */ return;\n}"
},
{
code: "function a() {\nvar b; /* comment */\nreturn;\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b; /* comment */\n\nreturn;\n}"
},
{
code: "function a() {\nvar b;\nreturn; //comment\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b;\n\nreturn; //comment\n}"
},
{
code: "function a() {\nvar b; return; //comment\n}",
errors: ["Expected newline before return statement."],
output: "function a() {\nvar b; \n\nreturn; //comment\n}"
}
]
});