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.
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
Update: Added generators option to func-names (fixes #9511) #10697
Changes from 1 commit
109faf2
13a8c74
7d845ae
67867f7
0950db3
df0be72
2f087c5
c54e1f7
41b828a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,15 @@ module.exports = { | |
schema: [ | ||
{ | ||
enum: ["always", "as-needed", "never"] | ||
}, | ||
{ | ||
type: "object", | ||
properties: { | ||
generators: { | ||
type: "boolean" | ||
} | ||
}, | ||
additionalProperties: false | ||
} | ||
] | ||
}, | ||
|
@@ -44,6 +53,23 @@ module.exports = { | |
const never = context.options[0] === "never"; | ||
const asNeeded = context.options[0] === "as-needed"; | ||
|
||
// defaults to true (func-name required) for "always" and "as-needed", false (func-name not required) for "never". | ||
let requireNamedGenerators = !never; | ||
|
||
if (context.options[1] && context.options[1].hasOwnProperty("generators")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is going to pass linting as is-- I think we now require |
||
switch (context.options[0]) { | ||
case "always": | ||
case "as-needed": | ||
requireNamedGenerators = context.options[1].generators; | ||
break; | ||
case "never": | ||
requireNamedGenerators = !context.options[1].generators; | ||
|
||
// no default | ||
} | ||
|
||
} | ||
|
||
/** | ||
* Determines whether the current FunctionExpression node is a get, set, or | ||
* shorthand method in an object literal or a class. | ||
|
@@ -79,6 +105,32 @@ module.exports = { | |
(parent.type === "AssignmentPattern" && parent.right === node); | ||
} | ||
|
||
/** | ||
* Reports that an unnamed function should be named | ||
* @param {ASTNode} node - The node to report in the event of an error. | ||
* @returns {void} | ||
*/ | ||
function reportUnexpectedUnnamedFunction(node) { | ||
context.report({ | ||
node, | ||
message: "Unexpected unnamed {{name}}.", | ||
data: { name: astUtils.getFunctionNameWithKind(node) } | ||
}); | ||
} | ||
|
||
/** | ||
* Reports that a named function should be unnamed | ||
* @param {ASTNode} node - The node to report in the event of an error. | ||
* @returns {void} | ||
*/ | ||
function reportUnexpectedNamedFunction(node) { | ||
context.report({ | ||
node, | ||
message: "Unexpected named {{name}}.", | ||
data: { name: astUtils.getFunctionNameWithKind(node) } | ||
}); | ||
} | ||
|
||
return { | ||
"FunctionExpression:exit"(node) { | ||
|
||
|
@@ -90,23 +142,22 @@ module.exports = { | |
} | ||
|
||
const hasName = Boolean(node.id && node.id.name); | ||
const name = astUtils.getFunctionNameWithKind(node); | ||
|
||
if (never) { | ||
if (hasName) { | ||
context.report({ | ||
node, | ||
message: "Unexpected named {{name}}.", | ||
data: { name } | ||
}); | ||
|
||
if (node.generator) { | ||
if (requireNamedGenerators && !hasName) { | ||
reportUnexpectedUnnamedFunction(node); | ||
} else if (!requireNamedGenerators && hasName) { | ||
reportUnexpectedNamedFunction(node); | ||
} | ||
} else { | ||
if (!hasName && (asNeeded ? !hasInferredName(node) : !isObjectOrClassMethod(node))) { | ||
context.report({ | ||
node, | ||
message: "Unexpected unnamed {{name}}.", | ||
data: { name } | ||
}); | ||
if (never) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the branching here makes this code a bit difficult to read; there is similar logic for both regular and generator functions, but the code ends up duplicated. Would it be possible to have something like a function (node) {
if (
node.generator &&
context.options.length > 1 &&
context.options[1].generators
) {
return context.options[1].generators;
} else {
return context.options[0] || "always";
}
} Then the |
||
if (hasName) { | ||
reportUnexpectedNamedFunction(node); | ||
} | ||
} else { | ||
if (!hasName && (asNeeded ? !hasInferredName(node) : !isObjectOrClassMethod(node))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is a bug here where the /* eslint func-names: [error, always, {generators: as-needed}] */
var foo = function*() {}; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry - I missed this case. I've updated the PR to handle it properly. |
||
reportUnexpectedUnnamedFunction(node); | ||
} | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of
true
andfalse
for this case (meaning "use the base option" and "invert the base option") is a bit unintuitive to me. Since there three possible base options (always
,as-needed
, andnever
) and only two possiblegenerators
options, a booleangenerators
option isn't able to express the full space of possible behaviors here. It also seems like the wording of "require generators to follow thefunc-names
rule" is self-referential since the option itself is describing what it means for a generator to follow thefunc-names
rule.What would you think about having the
generators
option be a string with the same possible values as the regular option? This seems like it could reduce the cognitive overhead when configuring the option, because thegenerators
option would behave the same way as the base option. For example:With this scheme, the default behavior of the
generators
option would be to fall back to the base option.Another possible change could be to make the first option an object, e.g.
With this setup, it might be clearer how the
generators
andbase
options interact with each other, without reading the documentation. (Note that we would still need to support the string option for backwards compatibility.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that makes sense. I'll look at updating the schema as suggested.