1263258 - [ESLint] Anonymous function / generator spacing rules

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Our current ESLint rules include:

"space-before-function-paren": [2, "never"],
"generator-star-spacing": [2, "after"],

So for regular functions the current rules say you should write:

function() {

but for generators (apparently only ESLint 2 checked anonymous generators, according the patches from bug 1257246), you must write:

function* () {

These two rules seem to be in conflict, as it seems strange that spacing would change based on whether it's a generator or not.  I do not personally care much whether there is or is not a space present, but I do think it would be nice to have the same rules for each of these two cases.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:pbro, what do you think about this?

Comment 2

[Patrick Brosset <:pbro>]

I agree we need some better consistency here.
We have 4 cases:

1 - anonymous function,
2 - named function,
3 - anonymous generator function,
4 - named generator function.

For 1, we've been writing this so far:
function() {}

For 2:
function foo() {}

And both of which are enforced by:
"space-before-function-paren": [2, "never"]

For 3, before eslint 2, we were writing this:
function*() {}
because this matched better with 1.

For 4, we've been writing this:
function* foo() {}
I guess because it felt weird to not have a space between the * and the function name, and because it felt more consistent with 2.

Now, with eslint 2, this rule:
"generator-star-spacing": [2, "after"],
also checks anonymous generator functions as you said, and so (3) became an error, and Dave had to do a massive search/replace to transform them to:
function* () {}

We could go back to having no space here, with this configuration:
"generator-star-spacing": [2, {"before": false, "after": false}],
With this, then function*() {} would be ok again, but we would have to replace all of our named generator functions to:
function*foo() {}

Here's my proposal: Always have a space before ( for anonymous functions and generators.
This way we can have this:

1) anon functions: function () {}
2) functions: function foo() {}
3) anon generators: function* () {}
4) generators: function* foo() {}

This will require:
"space-before-function-paren": [2, {"anonymous": "always", "named": "never"}],
"generator-star-spacing": [2, "after"],

Comment 3

[Patrick Brosset <:pbro>]

While we're making changes, maybe this would be better:
"generator-star-spacing": [2, "before"],
because of shorthands. This looks sort of weird I guess:

let foo = {
  * bar() {}
};

while this looks ok, at least to me:

let foo = {
  *bar() {}
};

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Patrick Brosset [:pbro] from comment #2)
> Here's my proposal: Always have a space before ( for anonymous functions and
> generators.
> This way we can have this:
> 
> 1) anon functions: function () {}
> 2) functions: function foo() {}
> 3) anon generators: function* () {}
> 4) generators: function* foo() {}
> 
> This will require:
> "space-before-function-paren": [2, {"anonymous": "always", "named":
> "never"}],
> "generator-star-spacing": [2, "after"],

I agree this seems consistent, except for the shorthand case...

(In reply to Patrick Brosset [:pbro] from comment #3)
> While we're making changes, maybe this would be better:
> "generator-star-spacing": [2, "before"],
> because of shorthands. This looks sort of weird I guess:
> 
> let foo = {
>   * bar() {}
> };
> 
> while this looks ok, at least to me:
> 
> let foo = {
>   *bar() {}
> };

Do we have many uses of "bare" generators in object shorthand form?  I guess it's a bit tricky to search for...  I feel like most generators in our code so far are wrapped in Task.async, so you can't use the shorthand form directly, and instead you end up with an anonymous generator:

let foo = {
  bar: Task.async(function* () {

  })
};

So, maybe we don't care so much about the shorthand case?  Or we could try to craft a custom rule that is like "generator-star-spacing": [2, "after"] but ignores the shorthand case...?

Comment 5

[Patrick Brosset <:pbro>]

You're right, we don't have many shorthands at all anyway, so shorthand generators, even less so.
So I think it'd be ok to live with * bar() {} for now. I would advise against creating more custom rules for now, they're not easy to maintain. If this really becomes a nuisance, we can try and submit a change on the upstream code instead.

So, if anyone is interested, the change here is to replace
"space-before-function-paren": [2, "never"],
with
"space-before-function-paren": [2, {"anonymous": "always", "named": "never"}],
in the file
/devtools/.eslintrc

And then do a massive search/replace on the whole devtools/ directory to replace 'function(' with 'function ('.
(note that you'd have to ignore the third-party scripts that we have here and there in the tree that we don't want eslint to check, and don't want to ever change).

I changed the rule locally and ran eslint, and ended up with 1069 errors

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1263258 - Require space before paren for anonymous functions. r=pbro

Review commit: https://reviewboard.mozilla.org/r/49109/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49109/

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ebb3d8489c8

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8745818 [details]
MozReview Request: Bug 1263258 - Require space before paren for anonymous functions. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49109/diff/1-2/

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=124ccb7b0614

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8745818 [details]
MozReview Request: Bug 1263258 - Require space before paren for anonymous functions. r=pbro

https://reviewboard.mozilla.org/r/49109/#review45993

Thanks Ryan!
Do you mind adding a new bullet point in https://wiki.mozilla.org/DevTools/CodingStandards#Code_style to summarize the spacing guidelines for all sorts of functions?
And maybe once updated, send this link to the mailing list too?

::: devtools/client/styleeditor/styleeditor-commands.js:65
(Diff revision 2)
> -    return gDevTools.showToolbox(target, "styleeditor").then(function(toolbox) {
> +    let toolboxOpened = gDevTools.showToolbox(target, "styleeditor");
> +    return toolboxOpened.then(function (toolbox) {

Ahah, I was scrolling around like crazy on mozreview to review this quickly because these were all mechanical changes, thinking to myself: "there's got to be at least one line where adding a space will make it go beyond the 80 char limit!". And there it is :)

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0a2e98e3e736

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Wiki updated and email sent.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a2e98e3e736

Comment 14

[Florian Quèze [:florian]]

The rule put in place here is inconsistent with what has been enforced in the rest of the tree :-(

See http://searchfox.org/mozilla-central/search?q=space-before-function-paren

This is painful when working on mass changes like bug 1331599.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Florian Quèze [:florian] from comment #14)
> The rule put in place here is inconsistent with what has been enforced in
> the rest of the tree :-(
> 
> See http://searchfox.org/mozilla-central/search?q=space-before-function-paren
> 
> This is painful when working on mass changes like bug 1331599.

In bug 1443081, we have now flipped DevTools back to Mozilla style for function spacing, so this pain should be gone now.

Comment 16

[Florian Quèze [:florian]]

Nice, thanks!