Update: Added generators option to func-names (fixes #9511)

[OscarBarrett]

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 for generators 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?

[jsf-clabot]

All committers have signed the CLA.

[not-an-aardvark]

Thanks for the pull request! I left a few suggestions for how the option schema can be improved.

[not-an-aardvark]

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.)

[OscarBarrett]

Thanks, that makes sense. I'll look at updating the schema as suggested.

[platinumazure]

I hate to be a pain about the schema, but would it be possible to use a schema like ["always", { generators: "as-needed" }]? This is much closer to how many other core rules are configured (default option first, then exceptions as an object option).

[OscarBarrett]

No problem! I've removed the alternative object schema.

[platinumazure]

Thank you @OscarBarrett! I'm hoping to find time to review in the next couple of days.

[platinumazure]

LGTM, thanks! Left one potential nitpick. (I'll need to restart the Travis job since I think this might no longer pass linting.)

[platinumazure]

Not sure if this is going to pass linting as is-- I think we now require Object.prototype.hasOwnProperty.call.

[not-an-aardvark]

Thanks for continuing to work on this. I think the generators: as-needed option isn't working correctly with the current implementation.

[not-an-aardvark]

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*() {};

[OscarBarrett]

Sorry - I missed this case. I've updated the PR to handle it properly.

[kaicataldo]

@not-an-aardvark Friendly ping - have your comments been addressed?

[not-an-aardvark]

This mostly looks good, I just have a suggestion for how to refactor it slightly to make it easier to read.

[not-an-aardvark]

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.)

[not-an-aardvark]

Just one more small suggestion, but this mostly looks very good.

[not-an-aardvark]

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).

[OscarBarrett]

Updated - looks like one of the travis builds stalled though.

[not-an-aardvark]

Looks good to me, thanks!