Skip to content

Commit

Permalink
Update: Improve array-callback-return report message (#13395)
Browse files Browse the repository at this point in the history
* Update: Improve array-callback-return report (explains why)
The array-callback-return rule should explain that a return value is required because the surrounding method requires it. This makes it clear that eg. .filter() expects the passed function to return something, rather than a general expectation of that function.

The error messages for expectedAtEnd/Inside now read 'Method .{{arrayMethodName}}() expected a value to be returned at the end of {{name}}.' and 'Method .{{arrayMethodName}}() expected a return value from {{name}}.'

* Update lib/rules/array-callback-return.js

Co-authored-by: Kai Cataldo <kai@kaicataldo.com>

* Update lib/rules/array-callback-return.js

Co-authored-by: Kai Cataldo <kai@kaicataldo.com>

* Update: Improve array-callback-return report (explains why) patch
* Made tests match proposed new message.

* fix: forEach message, .from Owner
* forEach has a message, and describes why no return value is expected.
* present tense ('Array.from expects no' vs 'Array.from expected no')
* Added explicit check of the forEach message

* fix: cleaner production of full-qyalified method name

Co-authored-by: Kai Cataldo <kai@kaicataldo.com>
  • Loading branch information
mrflip and kaicataldo committed Jul 3, 2020
1 parent 3f51930 commit ff5317e
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 109 deletions.
31 changes: 21 additions & 10 deletions lib/rules/array-callback-return.js
Expand Up @@ -9,8 +9,6 @@
// Requirements
//------------------------------------------------------------------------------

const lodash = require("lodash");

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

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -43,6 +41,19 @@ function isTargetMethod(node) {
);
}

/**
* Returns a human-legible description of an array method
* @param {string} arrayMethodName A method name to fully qualify
* @returns {string} the method name prefixed with `Array.` if it is a class method,
* or else `Array.prototype.` if it is an instance method.
*/
function fullMethodName(arrayMethodName) {
if (["from", "of", "isArray"].includes(arrayMethodName)) {
return "Array.".concat(arrayMethodName);
}
return "Array.prototype.".concat(arrayMethodName);
}

/**
* Checks whether or not a given node is a function expression which is the
* callback of an array method, returning the method name.
Expand Down Expand Up @@ -153,10 +164,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 +213,13 @@ module.exports = {
}

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

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

0 comments on commit ff5317e

Please sign in to comment.