Update: Add configurability to generator star spacing #8985

EthanRutherford · 2017-07-22T05:55:45Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] 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:

#8918

What changes did you make? (Give an overview)
Added configurability to generator star spacing.
Users of the rule will now be able to override the behavior of this rule per method type.
The types of methods allowing overrides are named function declarations, anonymous function declarations, function property shorthand/method declarations, and static method declarations.

Is there anything you'd like reviewers to focus on?
Let me know if there's a better way to provide documentation on the new functionality without drastically increasing the length of that document.
Also, let me know if there's anything that I missed that needs to be updated.

jsf-clabot · 2017-07-22T05:55:46Z

All committers have signed the CLA.

eslintbot · 2017-07-22T05:55:46Z

Thanks for the pull request, @EthanRutherford! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

mention-bot · 2017-07-22T05:55:49Z

@EthanRutherford, thanks for your PR! By analyzing the history of the files in this pull request, we identified @not-an-aardvark, @btmills and @mysticatea to be potential reviewers.

eslintbot · 2017-07-22T06:01:22Z

LGTM

aladdin-add · 2017-07-23T22:11:19Z

docs/rules/generator-star-spacing.md

@@ -85,7 +104,7 @@ Examples of **correct** code for this rule with the `"before"` option:
 /*eslint generator-star-spacing: ["error", {"before": true, "after": false}]*/
 /*eslint-env es6*/

-function *generator() {}
+function *generator() {};


nitpick: I don't think these semi are necessary.

True enough.. I added them for consistency, though I realize now that these didn't have them before because they're function declarations.

eslintbot · 2017-07-24T00:43:31Z

LGTM

eslintbot · 2017-07-24T00:47:30Z

LGTM

eslintbot · 2017-07-24T00:49:38Z

LGTM

EthanRutherford · 2017-07-27T21:37:42Z

Could I get some review on this?

not-an-aardvark · 2017-08-03T03:24:53Z

Hi, sorry about the lack of a response here. I think the holdup is mainly because #8918 is not accepted yet (it still has the "evaluating" label). We generally accept enhancements only after the team reaches consensus that they should be added (by having 3 👍s from team members on the issue, and having one team member willing to champion the proposal). The goal of this is to only accept enhancements that are clearly justified and generally useful (in order to ensure ESLint is maintainable in the long term, we can't accept all enhancements that people propose). Unfortunately, this does sometimes slow down the process of accepting enhancements.

EthanRutherford · 2017-08-03T22:02:19Z

Ah, I see. Well, hopefully we can get some more eyes on the issue soon. I think it's a pretty important enhancement, as the core of the issue is a conflict between the current implementation of generator-star-spacing and space-before-paren.

not-an-aardvark · 2017-08-16T16:39:26Z

Marking this PR as accepted because the corresponding issue has reached consensus.

platinumazure

Some nitpicks about the documentation, otherwise LGTM. Thanks!

platinumazure · 2017-08-16T17:53:51Z

docs/rules/generator-star-spacing.md

@@ -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


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

platinumazure · 2017-08-16T17:54:25Z

docs/rules/generator-star-spacing.md

@@ -137,6 +156,27 @@ var anonymous = function*() {};
 var shorthand = { *generator() {} };
 ```

+Examples of **correct** code for this rule with overrides present


Could this line end with a colon?

eslintbot · 2017-08-16T18:27:19Z

LGTM

not-an-aardvark

Thanks for the pull request! I left some comments.

not-an-aardvark · 2017-08-16T18:53:55Z

docs/rules/generator-star-spacing.md

@@ -137,6 +156,27 @@ var anonymous = function*() {};
 var shorthand = { *generator() {} };
 ```

+Examples of **correct** code for this rule with overrides present:


Can you add examples of incorrect code with the overrides?

not-an-aardvark · 2017-08-16T18:58:54Z

docs/rules/generator-star-spacing.md

+* `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


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.

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.

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.

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.

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.

not-an-aardvark · 2017-08-16T19:00:47Z

docs/rules/generator-star-spacing.md

+    "anonymous": "neither",
+    "method": "neither",
+    "static": {"before": true, "after": true}
+}]


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.

not-an-aardvark · 2017-08-16T19:05:19Z

lib/rules/generator-star-spacing.js

+                                            after: { type: "boolean" }
+                                        },
+                                        additionalProperties: false
+                                    }


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

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

not-an-aardvark · 2017-08-16T19:07:36Z

lib/rules/generator-star-spacing.js

+            return typeof option === "string" ? optionDefinitions[option] : {
+                before: "before" in option ? option.before : defaults.before,
+                after: "after" in option ? option.after : defaults.after
+            };


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

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

not-an-aardvark · 2017-08-16T19:23:55Z

tests/lib/rules/generator-star-spacing.js

+        {
+            code: "class Foo { * foo(){} }",
+            options: [{ before: false, after: false, method: "both" }]
+        },


Can you add tests for a method on an object?

({ *foo() {} })

not-an-aardvark · 2017-08-16T19:24:37Z

tests/lib/rules/generator-star-spacing.js

+        {
+            code: "class Foo { static * foo(){} }",
+            options: [{ before: false, after: false, static: "both" }]
+        },


Can you add tests where a specific override is used in object form (e.g. named: {before: true, after: false})?

not-an-aardvark · 2017-08-16T19:26:45Z

tests/lib/rules/generator-star-spacing.js

+        {
+            code: "class Foo { static * foo(){} }",
+            options: [{ before: false, after: false, static: "both" }]
+        },


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" }] }

not-an-aardvark · 2017-08-16T19:30:59Z

tests/lib/rules/generator-star-spacing.js

+        {
+            code: "class Foo { static * foo(){} }",
+            options: [{ before: false, after: false, static: "both" }]
+        },


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" }] }

not-an-aardvark · 2017-08-16T19:34:17Z

tests/lib/rules/generator-star-spacing.js

+        {
+            code: "class Foo { static * foo(){} }",
+            options: [{ before: false, after: false, static: "both" }]
+        },


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 } }] }

eslintbot · 2017-08-16T20:59:38Z

LGTM

EthanRutherford · 2017-08-16T22:23:51Z

@not-an-aardvark @platinumazure, code review comments have been addressed. Let me know if there's anything else that needs attention.

not-an-aardvark

Thanks, this looks good in general -- I just have a few minor suggestions.

not-an-aardvark · 2017-08-16T22:28:32Z

docs/rules/generator-star-spacing.md

+class Class { static * method() {} }
+```
+
+Examples of **incorrect** code for this rule with overrides present:


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

not-an-aardvark · 2017-08-16T22:33:48Z

lib/rules/generator-star-spacing.js

@@ -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"


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

not-an-aardvark · 2017-08-16T22:44:18Z

lib/rules/generator-star-spacing.js

            }

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


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.

eslintbot · 2017-08-16T23:06:27Z

LGTM

EthanRutherford · 2017-08-16T23:27:44Z

Thanks for the feedback. I think I've addressed everything now.

not-an-aardvark

Looks good to me. Thanks!

not-an-aardvark · 2017-08-31T03:44:24Z

@platinumazure Would you mind confirming whether your comments have been addressed? If so, I think we can merge this.

platinumazure

Sorry for the delay. LGTM, thanks for contributing!

EthanRutherford changed the title ~~Add configurability to generator star spacing~~ Update: Add configurability to generator star spacing Jul 22, 2017

Update: Add configurability to generator star spacing

d1eddb2

EthanRutherford force-pushed the GeneratorStarSpacing branch from d5b8788 to d1eddb2 Compare July 22, 2017 06:01

aladdin-add reviewed Jul 23, 2017

View reviewed changes

not-an-aardvark added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules labels Jul 24, 2017

EthanRutherford force-pushed the GeneratorStarSpacing branch from d3416fc to f4286f0 Compare July 24, 2017 00:47

Remove unnecessary semicolons

bcd64ee

EthanRutherford force-pushed the GeneratorStarSpacing branch from f4286f0 to bcd64ee Compare July 24, 2017 00:49

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Aug 16, 2017

platinumazure requested changes Aug 16, 2017

View reviewed changes

Add missing punctuation in documentation

fdad986

not-an-aardvark suggested changes Aug 16, 2017

View reviewed changes

Address review

1c638ed

not-an-aardvark suggested changes Aug 16, 2017

View reviewed changes

Address code review

5071a45

not-an-aardvark approved these changes Aug 16, 2017

View reviewed changes

not-an-aardvark requested a review from platinumazure August 24, 2017 18:25

platinumazure approved these changes Aug 31, 2017

View reviewed changes

not-an-aardvark merged commit 8fbaf0a into eslint:master Aug 31, 2017

eslint-deprecated bot locked and limited conversation to collaborators Feb 28, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 28, 2018

Update: Add configurability to generator star spacing #8985

Update: Add configurability to generator star spacing #8985

Conversation

EthanRutherford commented Jul 22, 2017

jsf-clabot commented Jul 22, 2017 • edited

eslintbot commented Jul 22, 2017

mention-bot commented Jul 22, 2017

eslintbot commented Jul 22, 2017

aladdin-add Jul 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jul 24, 2017

eslintbot commented Jul 24, 2017

eslintbot commented Jul 24, 2017

EthanRutherford commented Jul 27, 2017

not-an-aardvark commented Aug 3, 2017

EthanRutherford commented Aug 3, 2017

not-an-aardvark commented Aug 16, 2017

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Aug 16, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark Aug 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Aug 16, 2017

EthanRutherford commented Aug 16, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Aug 16, 2017

EthanRutherford commented Aug 16, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark commented Aug 31, 2017

platinumazure left a comment

Choose a reason for hiding this comment

jsf-clabot commented Jul 22, 2017 •

edited

aladdin-add Jul 23, 2017 •

edited

not-an-aardvark Aug 16, 2017 •

edited