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 2 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
40 changes: 40 additions & 0 deletions docs/rules/generator-star-spacing.md
View file
Open in desktop
Expand Up @@ -75,6 +75,25 @@ An example of shorthand configuration:
"generator-star-spacing": ["error", "after"]
```

Additionally, this rule allows further configurability via overrides per function type
Copy link
Member

Choose a reason for hiding this comment

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

Could this line end with a punctuation mark (either period/full stop or colon)?


* `named` provides overrides for named functions
* `anonymous` provides overrides for anonymous functions
* `method` provides overrides for class methods or property function shorthand
* `static` provides overrides for static class methods
Copy link
Member

Choose a reason for hiding this comment

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

I noticed there's an additional option for static here, which wasn't in the original proposal in #8918. I'm wondering if that design is the best choice. static methods are also methods, so it seems like this could cause confusion if someone configures the method option. Additionally, it seems like this could create the need for several more options when async generators are introduced (e.g. will we have asyncStatic as well as asyncMethod?)

At the moment I'm thinking the static option should be left out for now and combined with method unless there's a compelling need for it, but I'm open to discussion on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included it simply because it is, technically, its own distinct case. I could imagine someone wishing to have methods as

class Foo {
    ...
    *method() {}
}

but static methods as

class Bar {
    ...
    static* method() {}
}

I don't necessarily feel strongly about it, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One possible solution would be to support "static" as an override to the "method" override.. an override of an override. Starts to make the schema pretty complex, though.

Copy link
Member

Choose a reason for hiding this comment

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

Given that the "before" option is currently ignored for methods anyway, could we just not ignore it for static methods? That way people can use the method option for both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not opposed, though the case I described above would not be possible. I don't expect to see anyone necessarily asking for that specific combination of configs, so we can probably ignore that.


An example of a configuration with overrides:

```json
"generator-star-spacing": ["error", {
"before": false,
"after": true,
"anonymous": "neither",
"method": "neither",
"static": {"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.

```

## Examples

### before
Expand Down Expand Up @@ -137,6 +156,27 @@ 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.

Could this line end with a colon?


```js
/*eslint generator-star-spacing: ["error", {
"before": false,
"after": true,
"anonymous": "neither",
"method": "neither",
"static": {"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
132 changes: 113 additions & 19 deletions lib/rules/generator-star-spacing.js
View file
Open in desktop
Expand Up @@ -29,7 +29,67 @@ module.exports = {
type: "object",
properties: {
before: { type: "boolean" },
after: { type: "boolean" }
after: { type: "boolean" },
named: {
oneOf: [
{
enum: ["before", "after", "both", "neither"]
},
{
type: "object",
properties: {
before: { type: "boolean" },
after: { type: "boolean" }
},
additionalProperties: false
}
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: This schema is repeated several times. Can you extract the object into a constant?

const SPECIFIC_CONFIG_SCHEMA = {
    oneOf: [
        // ...
    ]
};

]
},
anonymous: {
oneOf: [
{
enum: ["before", "after", "both", "neither"]
},
{
type: "object",
properties: {
before: { type: "boolean" },
after: { type: "boolean" }
},
additionalProperties: false
}
]
},
method: {
oneOf: [
{
enum: ["before", "after", "both", "neither"]
},
{
type: "object",
properties: {
before: { type: "boolean" },
after: { type: "boolean" }
},
additionalProperties: false
}
]
},
static: {
oneOf: [
{
enum: ["before", "after", "both", "neither"]
},
{
type: "object",
properties: {
before: { type: "boolean" },
after: { type: "boolean" }
},
additionalProperties: false
}
]
}
},
additionalProperties: false
}
Expand All @@ -40,16 +100,41 @@ 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] : {
before: "before" in option ? option.before : defaults.before,
after: "after" in option ? option.after : defaults.after
};
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: I think this line would be more readable if it used Object.assign:

return typeof option === "string"
    ? optionDefinitions[options]
    : 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),
static: optionToDefinition(option.static, defaults)
};
}(context.options[0]));

const sourceCode = context.getSourceCode();
Expand Down Expand Up @@ -79,17 +164,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 +204,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 +214,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.type === "FunctionExpression" && (node.parent.type === "MethodDefinition" || node.parent.type === "Property")) {
kind = prevToken.value === "static" ? "static" : "method";
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this check for methods is correct. It's possible for objects to have function properties that aren't methods:

var foo = {
  bar: function* () {
  }
};

That should fall under anonymous, not method.

Instead, I think it would be better to check the .method property of the parent node when node.parent.type === "Property". The method property will be true if the node is a method, and false otherwise.

} else if (nextToken.value === "(") {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to check for anonymous functions by verifying that node.id is null, rather than checking that the next token is an opening paren.

kind = "anonymous";
}

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

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

return {
Expand Down
65 changes: 65 additions & 0 deletions tests/lib/rules/generator-star-spacing.js
View file
Open in desktop
Expand Up @@ -404,6 +404,24 @@ ruleTester.run("generator-star-spacing", rule, {
options: [{ before: false, after: false }]
},

// full configurability
{
code: "function * foo(){}",
options: [{ before: false, after: false, named: "both" }]
},
{
code: "var foo = function * (){};",
options: [{ before: false, after: false, anonymous: "both" }]
},
{
code: "class Foo { * foo(){} }",
options: [{ before: false, after: false, method: "both" }]
},
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 tests for a method on an object?

({ *foo() {} })

{
code: "class Foo { static * foo(){} }",
options: [{ before: false, after: false, static: "both" }]
},
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 tests where a specific override is used in object form (e.g. named: {before: true, after: false})?

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 tests where an override is used, but that override doesn't apply to the given code?

e.g.

{
    code: "function* foo() {}",
    options: [{ before: false, after: true, method: "before" }]
}

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 tests where an override is used and top-level before and after are not used? In that case, the rule default should be used ("before").

{
    code: "function *foo() {}",
    options: [{ method: "after" }]
}

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 tests where the object option is specified, but only one of before or after is provided? Then I would expect the other option to fall back to the default.

{
    code: "function * foo(){}",
    options: [{ before: true, after: false, named: { after: true } }]
}


// https://github.com/eslint/eslint/issues/7101#issuecomment-246080531
{
code: "async function foo() { }",
Expand Down Expand Up @@ -1168,6 +1186,53 @@ ruleTester.run("generator-star-spacing", rule, {
message: "Unexpected space after *.",
type: "Punctuator"
}]
},

// full configurability
{
code: "function*foo(){}",
output: "function * foo(){}",
options: [{ before: false, after: false, named: "both" }],
errors: [{
message: "Missing space before *.",
type: "Punctuator"
}, {
message: "Missing space after *.",
type: "Punctuator"
}]
},
{
code: "var foo = function*(){};",
output: "var foo = function * (){};",
options: [{ before: false, after: false, anonymous: "both" }],
errors: [{
message: "Missing space before *.",
type: "Punctuator"
}, {
message: "Missing space after *.",
type: "Punctuator"
}]
},
{
code: "class Foo { *foo(){} }",
output: "class Foo { * foo(){} }",
options: [{ before: false, after: false, method: "both" }],
errors: [{
message: "Missing space after *.",
type: "Punctuator"
}]
},
{
code: "class Foo { static*foo(){} }",
output: "class Foo { static * foo(){} }",
options: [{ before: false, after: false, static: "both" }],
errors: [{
message: "Missing space before *.",
type: "Punctuator"
}, {
message: "Missing space after *.",
type: "Punctuator"
}]
}

]
Expand Down