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: Improve array-callback-return report message #13395

Merged
merged 6 commits into from Jul 3, 2020
Merged

Update: Improve array-callback-return report message #13395

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e526893
Update: Improve array-callback-return report (explains why)
mrflip Jun 5, 2020
e6f640f
Update lib/rules/array-callback-return.js
mrflip Jun 15, 2020
65374e1
Update lib/rules/array-callback-return.js
mrflip Jun 15, 2020
0cdf1a8
Update: Improve array-callback-return report (explains why) patch
mrflip Jun 15, 2020
71847d4
fix: forEach message, .from Owner
mrflip Jun 28, 2020
2e349c2
fix: cleaner production of full-qyalified method name
mrflip Jun 28, 2020
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
21 changes: 11 additions & 10 deletions lib/rules/array-callback-return.js
View file
Open in desktop
Expand Up @@ -9,8 +9,6 @@
// Requirements
//------------------------------------------------------------------------------

const lodash = require("lodash");

const astUtils = require("./utils/ast-utils");

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -153,10 +151,10 @@ module.exports = {
],

messages: {
expectedAtEnd: "Expected to return a value at the end of {{name}}.",
expectedInside: "Expected to return a value in {{name}}.",
expectedReturnValue: "{{name}} expected a return value.",
expectedNoReturnValue: "{{name}} did not expect a return value."
expectedAtEnd: "{{arrayMethodName}}() expects a value to be returned at the end of {{name}}.",
expectedInside: "{{arrayMethodName}}() expects a return value from {{name}}.",
expectedReturnValue: "{{arrayMethodName}}() expects a return value from {{name}}.",
expectedNoReturnValue: "{{arrayMethodName}}() expects no useless return value from {{name}}."
}
},

Expand Down Expand Up @@ -202,14 +200,16 @@ module.exports = {
}

if (messageId) {
let name = astUtils.getFunctionNameWithKind(node);
const name = astUtils.getFunctionNameWithKind(node);

const methodOwner = (funcInfo.arrayMethodName === "from" ? "Array." : "Array.prototype.");
const arrayMethodName = methodOwner.concat(funcInfo.arrayMethodName);

name = messageId === "expectedNoReturnValue" ? lodash.upperFirst(name) : name;
context.report({
node,
loc: astUtils.getFunctionHeadLoc(node, sourceCode),
messageId,
data: { name }
data: { name, arrayMethodName }
});
}
}
Expand Down Expand Up @@ -273,7 +273,8 @@ module.exports = {
node,
messageId,
data: {
name: lodash.upperFirst(astUtils.getFunctionNameWithKind(funcInfo.node))
name: astUtils.getFunctionNameWithKind(funcInfo.node),
arrayMethodName: "Array.prototype.".concat(funcInfo.arrayMethodName)
Copy link
Member

Choose a reason for hiding this comment

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

This path can also report Array.from, e.g.:

/* eslint array-callback-return: error */

// it shouldn't be "Array.prototype.from() expects a return value from arrow function"
Array.from(foo, () => { return; });

Maybe we could make a function that gets funcInfo.arrayMethodName and returns its "full name", and then use that function here and from the other context.report?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, those lines felt fishy and a function is the right answer. I went ahead and included all class methods (of, isArray, from) in case the function was ever lifted somewhere else. Pushed a commit with this change.

}
});
}
Expand Down