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: Add configurability to generator star spacing #8985

Merged
merged 5 commits into from Aug 31, 2017
Merged

Update: Add configurability to generator star spacing #8985

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d1eddb2
Update: Add configurability to generator star spacing
EthanRutherford Jul 22, 2017
bcd64ee
Remove unnecessary semicolons
EthanRutherford Jul 24, 2017
fdad986
Add missing punctuation in documentation
EthanRutherford Aug 16, 2017
1c638ed
Address review
EthanRutherford Aug 16, 2017
5071a45
Address code review
EthanRutherford Aug 16, 2017
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
61 changes: 61 additions & 0 deletions docs/rules/generator-star-spacing.md
View file
Open in desktop
Expand Up @@ -75,6 +75,27 @@ An example of shorthand configuration:
"generator-star-spacing": ["error", "after"]
```

Additionally, this rule allows further configurability via overrides per function type.

* `named` provides overrides for named functions
* `anonymous` provides overrides for anonymous functions
* `method` provides overrides for class methods or property function shorthand

An example of a configuration with overrides:

```json
"generator-star-spacing": ["error", {
"before": false,
"after": true,
"anonymous": "neither",
"method": {"before": true, "after": true}
}]
Copy link
Member

Choose a reason for hiding this comment

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

Can you update the documentation to explain what this configuration does? In particular, I'm unsure about what happens when "before" and "after" are used as top-level options alongside the more configurable options.

```

In the example configuration above, the top level "before" and "after" options define the default behavior of
the rule, while the "anonymous", "method", and "static" options override the default behavior.
Overrides can be either an object with "before" and "after", or a shorthand string as above.

## Examples

### before
Expand Down Expand Up @@ -137,6 +158,46 @@ var anonymous = function*() {};
var shorthand = { *generator() {} };
```

Examples of **correct** code for this rule with overrides present:
Copy link
Member

Choose a reason for hiding this comment

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

Can you add examples of incorrect code with the overrides?


```js
/*eslint generator-star-spacing: ["error", {
"before": false,
"after": true,
"anonymous": "neither",
"method": {"before": true, "after": true}
}]*/
/*eslint-env es6*/

function* generator() {}

var anonymous = function*() {};

var shorthand = { * generator() {} };

class Class { static * method() {} }
```

Examples of **incorrect** code for this rule with overrides present:
Copy link
Member

Choose a reason for hiding this comment

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

Very minor nitpick: In most of the documentation, incorrect code is listed before correct code. It would be nice to also do that here.


```js
/*eslint generator-star-spacing: ["error", {
"before": false,
"after": true,
"anonymous": "neither",
"method": {"before": true, "after": true}
}]*/
/*eslint-env es6*/

function * generator() {}

var anonymous = function* () {};

var shorthand = { *generator() {} };

class Class { static* method() {} }
```

## When Not To Use It

If your project will not be using generators or you are not concerned with spacing consistency, you do not need this rule.
Expand Down
89 changes: 70 additions & 19 deletions lib/rules/generator-star-spacing.js
View file
Open in desktop
Expand Up @@ -9,6 +9,22 @@
// Rule Definition
//------------------------------------------------------------------------------

const OVERRIDE_SCHEMA = {
oneOf: [
{
enum: ["before", "after", "both", "neither"]
},
{
type: "object",
properties: {
before: { type: "boolean" },
after: { type: "boolean" }
},
additionalProperties: false
}
]
};

module.exports = {
meta: {
docs: {
Expand All @@ -29,7 +45,10 @@ module.exports = {
type: "object",
properties: {
before: { type: "boolean" },
after: { type: "boolean" }
after: { type: "boolean" },
named: OVERRIDE_SCHEMA,
anonymous: OVERRIDE_SCHEMA,
method: OVERRIDE_SCHEMA
},
additionalProperties: false
}
Expand All @@ -40,16 +59,39 @@ module.exports = {

create(context) {

const mode = (function(option) {
if (!option || typeof option === "string") {
return {
before: { before: true, after: false },
after: { before: false, after: true },
both: { before: true, after: true },
neither: { before: false, after: false }
}[option || "before"];
const optionDefinitions = {
before: { before: true, after: false },
after: { before: false, after: true },
both: { before: true, after: true },
neither: { before: false, after: false }
};

/**
* Returns resolved option definitions based on an option and defaults
*
* @param {any} option - The option object or string value
* @param {Object} defaults - The defaults to use if options are not present
* @returns {Object} the resolved object definition
*/
function optionToDefinition(option, defaults) {
if (!option) {
return defaults;
}
return option;

return typeof option === "string"
? optionDefinitions[option]
: Object.assign({}, defaults, option);
}

const modes = (function(option) {
option = option || {};
const defaults = optionToDefinition(option, optionDefinitions.before);

return {
named: optionToDefinition(option.named, defaults),
anonymous: optionToDefinition(option.anonymous, defaults),
method: optionToDefinition(option.method, defaults)
};
}(context.options[0]));

const sourceCode = context.getSourceCode();
Expand Down Expand Up @@ -79,17 +121,19 @@ module.exports = {

/**
* Checks the spacing between two tokens before or after the star token.
*
* @param {string} kind Either "named", "anonymous", "method", or "static"
Copy link
Member

Choose a reason for hiding this comment

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

This comment should probably be updated to remove "static".

* @param {string} side Either "before" or "after".
* @param {Token} leftToken `function` keyword token if side is "before", or
* star token if side is "after".
* @param {Token} rightToken Star token if side is "before", or identifier
* token if side is "after".
* @returns {void}
*/
function checkSpacing(side, leftToken, rightToken) {
if (!!(rightToken.range[0] - leftToken.range[1]) !== mode[side]) {
function checkSpacing(kind, side, leftToken, rightToken) {
if (!!(rightToken.range[0] - leftToken.range[1]) !== modes[kind][side]) {
const after = leftToken.value === "*";
const spaceRequired = mode[side];
const spaceRequired = modes[kind][side];
const node = after ? leftToken : rightToken;
const type = spaceRequired ? "Missing" : "Unexpected";
const message = "{{type}} space {{side}} *.";
Expand Down Expand Up @@ -117,6 +161,7 @@ module.exports = {

/**
* Enforces the spacing around the star if node is a generator function.
*
* @param {ASTNode} node A function expression or declaration node.
* @returns {void}
*/
Expand All @@ -126,17 +171,23 @@ module.exports = {
}

const starToken = getStarToken(node);

// Only check before when preceded by `function`|`static` keyword
const prevToken = sourceCode.getTokenBefore(starToken);
const nextToken = sourceCode.getTokenAfter(starToken);

if (prevToken.value === "function" || prevToken.value === "static") {
checkSpacing("before", prevToken, starToken);
let kind = "named";

if (node.parent.type === "MethodDefinition" || (node.parent.type === "Property" && node.parent.method)) {
kind = "method";
} else if (!node.id) {
kind = "anonymous";
}

const nextToken = sourceCode.getTokenAfter(starToken);
// Only check before when preceded by `function`|`static` keyword
if (kind !== "method" || prevToken.value === "static") {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this check could be expressed in a better way. Conceptually, it seems like the case we care about isn't whether the value of the previous token happens to be static -- it's that we don't want to enforce spacing before the star when it's the first token of the given property.

class Foo {
  *bar() {} // '*' is the first token of the MethodDefinition
}

class Foo {
  static bar() {} // '*' is not the first token of the MethodDefinition (it's preceded by `static`)
}

// (experimental syntax)
class Foo {
  async *bar(){} // '*' is not the first token of the MethodDefinition (it's preceded by `async`)
}

So it might be better to express this statement as

if (!(kind === "method" && starToken === sourceCode.getFirstToken(node.parent))) {
    checkSpacing(kind, "before", prevToken, starToken);
}

This matters because although we don't explicitly support specific custom/experimental syntax in core rules, the rule will probably be run with various custom parsers, so it's useful to make the rule robust when dealing with unrecognized syntax in general. To that end, it helps to make rule's conditionals semantically correct so that they behave reasonably when they encounter syntax they don't know how to deal with.

checkSpacing(kind, "before", prevToken, starToken);
}

checkSpacing("after", starToken, nextToken);
checkSpacing(kind, "after", starToken, nextToken);
}

return {
Expand Down