1225289 - Create appropriate .eslintrc for eslint-plugin-mozilla folder

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

+++ This bug was initially created as a clone of Bug #1224735 +++

To avoid issues with people using older versions of node we should have a ruleset to avoid things that have recently been added to JavaScript.

These rules should work:
{
  // Extend from the shared list of defined globals for mochitests.
  "extends": "../../../../devtools/.eslintrc",

  "ecmaFeatures": {
    "arrowFunctions": false,
    "blockBindings": false,
    "destructuring": false,
    "regexYFlag": false,
    "regexUFlag": false,
    "templateStrings": false,
    "binaryLiterals": false,
    "octalLiterals": false,
    "unicodeCodePointEscapes": false,
    "defaultParams": false,
    "restParams": false,
    "forOf": false,
    "objectLiteralComputedProperties": false,
    "objectLiteralShorthandMethods": false,
    "objectLiteralShorthandProperties": false,
    "objectLiteralDuplicateProperties": false,
    "generators": false,
    "spread": false,
    "superInFunctions": false,
    "classes": false,
    "modules": true,
    "globalReturn": true
  }
}

We also need to get eslint to work from anywhere in the tree

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: MozReview Request: Bug 1225289 - Make eslint plugin code conform to .eslintrc r=pbrosset

Bug 1225289 - Create appropriate .eslintrc for eslint-plugin-mozilla folder r?=pbrosset

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #13)
> Comment on attachment 8690135 [details]
> MozReview Request: Bug 1225289 - Create appropriate .eslintrc for
> eslint-plugin-mozilla folder r?=pbrosset
>
> https://reviewboard.mozilla.org/r/25773/#review23291
>
> ::: testing/eslint-plugin-mozilla/lib/rules/.eslintrc:3
> (Diff revision 1)
> > +  "extends": "../../../../devtools/.eslintrc",
>
> Wait, what's this eslintrc for?
> Is it used by eslint to check the content of the
> testing/eslint-plugin-mozilla/lib/rules/?
> If yes, why are the custom mozilla rules enabled?
> And why does it extend from the devtools eslintrc file?
> The code around here is generic enough that any code in the tree should be
> able to use it, so there really shouldn't devtools mentioned here.

Removed

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8690775 [details]
MozReview Request: Bug 1225289 - Make eslint plugin code conform to .eslintrc r=pbrosset

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

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8690775 [details]
MozReview Request: Bug 1225289 - Make eslint plugin code conform to .eslintrc r=pbrosset

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

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8690775 [details]
MozReview Request: Bug 1225289 - Make eslint plugin code conform to .eslintrc r=pbrosset

https://reviewboard.mozilla.org/r/25947/#review23319

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: MozReview Request: Bug 1225289 - Create appropriate .eslintrc for eslint-plugin-mozilla folder r?=pbrosset

Bug 1225289 - Create appropriate .eslintrc for eslint-plugin-mozilla folder r?=pbrosset

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #13)
> Comment on attachment 8690135 [details]
> MozReview Request: Bug 1225289 - Create appropriate .eslintrc for
> eslint-plugin-mozilla folder r?=pbrosset
>
> https://reviewboard.mozilla.org/r/25773/#review23291
>
> ::: testing/eslint-plugin-mozilla/lib/rules/.eslintrc:3
> (Diff revision 1)
> > +  "extends": "../../../../devtools/.eslintrc",
>
> Wait, what's this eslintrc for?
> Is it used by eslint to check the content of the
> testing/eslint-plugin-mozilla/lib/rules/?
> If yes, why are the custom mozilla rules enabled?
> And why does it extend from the devtools eslintrc file?
> The code around here is generic enough that any code in the tree should be
> able to use it, so there really shouldn't devtools mentioned here.

Removed

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8690775 [details]
MozReview Request: Bug 1225289 - Make eslint plugin code conform to .eslintrc r=pbrosset

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

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

pbrosset: MozReview got it wrong... use this URL:
https://reviewboard.mozilla.org/r/25947/diff/3/

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8690793 [details]
MozReview Request: Bug 1225289 - Create appropriate .eslintrc for eslint-plugin-mozilla folder r?=pbrosset

https://reviewboard.mozilla.org/r/25955/#review23343

::: testing/eslint-plugin-mozilla/lib/rules/.eslintrc:2
(Diff revision 1)
> +  "rules": {

As discussed: I just don't think we should use this .eslintrc config file as a documentation for the rules. We already have docs in another directory.

::: testing/eslint-plugin-mozilla/lib/rules/.eslintrc:14
(Diff revision 1)
> +    "Buffer": true,

As someone not used to nodejs that much, I had no idea these were the nodejs globals. So at least add a comment about this.
__But__ eslint also has a way to enable the nodejs environment, without you having to list all of these:
```
{
    "env": {
        "node": true
    }
}
```

::: testing/eslint-plugin-mozilla/lib/rules/.eslintrc:34
(Diff revision 1)
> +  "ecmaFeatures": {

I think by default, eslint considers the code to be es5, so I don't see a need for specifically disabling these language features:
http://eslint.org/docs/user-guide/configuring#specifying-language-options

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://reviewboard.mozilla.org/r/25955/#review23343

> As someone not used to nodejs that much, I had no idea these were the nodejs globals. So at least add a comment about this.
> __But__ eslint also has a way to enable the nodejs environment, without you having to list all of these:
> ```
> {
>     "env": {
>         "node": true
>     }
> }
> ```

This misses a few globals so I am now using env.node and setting the missing globals.

I have also added some rules like no-undef etc. These rules are based on https://docs.npmjs.com/misc/coding-style with a few differences:
  * Commas should be at the end of a line.
  * Always use semicolons.
  * Functions should not have whitespace before params.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8690793 [details]
MozReview Request: Bug 1225289 - Create appropriate .eslintrc for eslint-plugin-mozilla folder r?=pbrosset

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

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8690775 [details]
MozReview Request: Bug 1225289 - Make eslint plugin code conform to .eslintrc r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25947/diff/3-4/

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8690793 [details]
MozReview Request: Bug 1225289 - Create appropriate .eslintrc for eslint-plugin-mozilla folder r?=pbrosset

https://reviewboard.mozilla.org/r/25955/#review23465

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/017a371f1c85
https://hg.mozilla.org/integration/fx-team/rev/2c7500d7c6de

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/017a371f1c85
https://hg.mozilla.org/mozilla-central/rev/2c7500d7c6de