If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[ESLint] Anonymous function / generator spacing rules

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Developer Tools
P3
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

unspecified
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [btpp-backlog])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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

Comment 1

2 years ago
:pbro, what do you think about this?
Flags: needinfo?(pbrosset)
(Assignee)

Updated

2 years ago
Priority: -- → P3
Whiteboard: [btpp-backlog]
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"],
Flags: needinfo?(pbrosset)
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() {}
};
(Assignee)

Comment 4

2 years ago
(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...?
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
Created attachment 8745818 [details]
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/
Attachment #8745818 - Flags: review?(pbrosset)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ebb3d8489c8
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/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=124ccb7b0614
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 :)
Attachment #8745818 - Flags: review?(pbrosset) → review+

Comment 11

a year ago
https://hg.mozilla.org/integration/fx-team/rev/0a2e98e3e736
Wiki updated and email sent.

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0a2e98e3e736
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee: nobody → jryans
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.
You need to log in before you can comment on or make changes to this bug.