-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Update: Added generators option to func-names (fixes #9511) #10697
Conversation
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 for the pull request! I left a few suggestions for how the option schema can be improved.
docs/rules/func-names.md
Outdated
* `"generators": true (default) | false` | ||
* `true`: require generators to follow the func-names rule. `"always"` or `"as-needed"` will require named generator functions, `"never"` will allow unnamed generator functions. | ||
* `false`: require generators to not follow the func-names rule. `"always"` or `"as-needed"` will allow unnamed generator functions, `"never"` will require named generator functions. | ||
|
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
and false
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
, and never
) and only two possible generators
options, a boolean generators
option isn't able to express the full space of possible behaviors here. It also seems like the wording of "require generators to follow the func-names
rule" is self-referential since the option itself is describing what it means for a generator to follow the func-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 the generators
option would behave the same way as the base option. For example:
{
"rules": {
"func-names": ["error", "never", {"generators": "as-needed"}]
}
}
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.
{
"rules": {
"func-names": [{"base": "never", "generators": "as-needed"}]
}
}
With this setup, it might be clearer how the generators
and base
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.
I hate to be a pain about the schema, but would it be possible to use a schema like |
No problem! I've removed the alternative object schema. |
Thank you @OscarBarrett! I'm hoping to find time to review in the next couple of days. |
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.
LGTM, thanks! Left one potential nitpick. (I'll need to restart the Travis job since I think this might no longer pass linting.)
lib/rules/func-names.js
Outdated
}, | ||
|
||
create(context) { | ||
const never = context.options[0] === "never"; | ||
const asNeeded = context.options[0] === "as-needed"; | ||
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 comment
The 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 Object.prototype.hasOwnProperty.call
.
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 for continuing to work on this. I think the generators: as-needed
option isn't working correctly with the current implementation.
lib/rules/func-names.js
Outdated
reportUnexpectedNamedFunction(node); | ||
} | ||
} else { | ||
if (!hasName && (asNeeded ? !hasInferredName(node) : !isObjectOrClassMethod(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.
I think there is a bug here where the as-needed
option never gets used for generators. For example, based on reading this I think the rule will incorrectly report an error for this code:
/* eslint func-names: [error, always, {generators: as-needed}] */
var foo = function*() {};
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.
Sorry - I missed this case. I've updated the PR to handle it properly.
@not-an-aardvark Friendly ping - have your comments been addressed? |
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.
This mostly looks good, I just have a suggestion for how to refactor it slightly to make it easier to read.
lib/rules/func-names.js
Outdated
messageId: "unnamed", | ||
data: { name } | ||
}); | ||
if (never) { |
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.
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 getConfigForNode
function that accepts a function AST node and returns the appropriate config for it? For example:
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 "FunctionExpression:exit"
listener would just perform the appropriate check based on the result of calling getConfigForNode
, without needing to have separate cases for when the given node is a generator. (This would also make the never
, asNeeded
, requireNamedGenerators
, and asNeededGenerators
options unnecessary.)
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.
Just one more small suggestion, but this mostly looks very good.
lib/rules/func-names.js
Outdated
if (!hasName && (asNeeded ? !hasInferredName(node) : !isObjectOrClassMethod(node))) { | ||
reportUnexpectedUnnamedFunction(node); | ||
} | ||
if (!hasName && (config === "as-needed" ? !hasInferredName(node) : !isObjectOrClassMethod(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.
Could this be simplified to something like:
} else if (config === "as-needed") {
if (!hasName && !hasInferredName(node)) {
reportUnexpectedUnnamedFunction(node);
}
} else {
if (!isObjectOrClassMethod(node)) {
reportUnexpectedUnnamedFunction(node);
}
}
In my opinion, the expression !hasName && (config === "as-needed" ? !hasInferredName(node) : !isObjectOrClassMethod(node))
is fairly difficult to read and understand (most of this complexity was already there before this change).
Updated - looks like one of the travis builds stalled though. |
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.
Looks good to me, thanks!
What is the purpose of this pull request? (put an "X" next to item)
[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
See #9511
What changes did you make? (Give an overview)
Added a
generators
object option to func-names. When a value forgenerators
is not provided the behavior for generator functions falls back to the base option.The
generators
object option has the following effect:"always"
require named generators"as-needed"
require named generators if the name cannot be assigned automatically in an ES6 environment. In practice this is everywhere."never"
disallow named generators where possible.Is there anything you'd like reviewers to focus on?