Closed Bug 1347712 Opened 6 years ago Closed 6 years ago

Add preset configs to eslint-plugin-mozilla

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Version:
3 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: pdehaan, Assigned: standard8)

References

Details

Attachments

(4 files)

Bug 1347712 - Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - structural changes.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1347712 - Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - automatically update .eslintrc.js test config files for new config locations.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1347712 - Move toolkit/.eslintrc.js rules into a 'recommended' set within eslint-plugin-mozilla.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1347712 - Add the tree-wide rules and config to the recommended eslint-plugin-mozilla config, to make it easier for outside projects.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
Re: https://github.com/mozilla-services/pageshot/issues/2365#issuecomment-286217069

It'd be super neat if we could do something simple like this in our other repos (that we expect to ultimately merge back into m-c):

```
# .eslintrc.yml
extends:
  - eslint:recommended
  - plugin:mozilla/recommended
```

I believe this is as "simple" as adding a `configs` Object key in the plugin with whatever custom configs we want. At a minimum, we should have "recommended", but you may also want custom configs for addons or webextensions, or browsers, or workers, or regular ES6 code, so we can inject the correct globals in the right contexts.
See eslint-plugin-react's config: https://github.com/yannickcr/eslint-plugin-react/blob/b64648521f9a7949eef4cd1a210768e906b4f3d1/index.js#L102-L140
Not sure if something like this is kosher:

```
  "configs": {
    "recommended": {
      "rules": {
        "mozilla/avoid-removeChild": "error",
        "mozilla/balanced-listeners": "error",
        "mozilla/import-globals": "error",
        "mozilla/import-headjs-globals": "error",
        "mozilla/mark-test-function-used": "error",
        "mozilla/no-aArgs": "error",
        "mozilla/no-cpows-in-tests": "error",
        "mozilla/no-single-arg-cu-import": "error",
        "mozilla/no-import-into-var-and-global": "error",
        "mozilla/no-useless-parameters": "error",
        "mozilla/no-useless-removeEventListener": "error",
        "mozilla/reject-importGlobalProperties": "error",
        // "mozilla/reject-some-requires": "warn",
        "mozilla/use-default-preference-values": "error",
        "mozilla/use-ownerGlobal": "error",
        "mozilla/var-only-at-top-level": "error",

        "brace-style": ["error", "1tbs"],
        "camelcase": "error",
        "comma-dangle": ["error", "never"],
        "comma-spacing": "error",
        "comma-style": ["error", "last"],
        "curly": ["error", "multi-line"],
        "handle-callback-err": ["error", "er"],
        "indent": ["error", 2, {"SwitchCase": 1}],
        "keyword-spacing": "error",
        "max-len": ["error", 80, 2],
        "no-multiple-empty-lines": ["error", {"max": 1}],
        "no-undef": "error",
        "no-undef-init": "error",
        "no-unexpected-multiline": "error",
        "object-curly-spacing": "off",
        "one-var": ["error", "never"],
        "operator-linebreak": ["error", "after"],
        "semi": ["error", "always"],
        "space-before-blocks": "error",
        "space-before-function-paren": ["error", "never"],
        "strict": ["error", "global"],
      },
    },
  },
```

It's pretty strict, but forces the user to explicitly disable/override rules they aren't interested in.
Blocks: 1347645
Assignee: nobody → standard8
Comment on attachment 8849120 [details]
Bug 1347712 - Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - structural changes.

These couple of patches move our testing configurations for eslint (mochitest, chrome, browser, xpcshell) into configs supplied by the plugin.

As a bonus, this gets rid of the relative paths mess in the .eslintrc.js files.

I'm also planning on moving at least the toolkit/.eslintrc.js config to a "recommended" configuration - however, I'd like to get feedback on this before I do more work.

(there may be other configs we should move, but I'd like to start looking at unifying what we have with the core toolkit config first & using it to highlight differences).
Attachment #8849120 - Flags: feedback?(jaws)
Attachment #8849120 - Flags: feedback?(dtownsend)
Attachment #8849121 - Flags: feedback?(jaws)
Attachment #8849121 - Flags: feedback?(dtownsend)
Comment on attachment 8849120 [details]
Bug 1347712 - Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - structural changes.

Yes, this is awesome
Attachment #8849120 - Flags: feedback?(dtownsend) → feedback+
Attachment #8849121 - Flags: feedback?(dtownsend) → feedback+
(In reply to Mark Banner (:standard8) from comment #4)
> I'm also planning on moving at least the toolkit/.eslintrc.js config to a
> "recommended" configuration - however, I'd like to get feedback on this
> before I do more work.

Yeah I think this is a great idea!
Comment on attachment 8849120 [details]
Bug 1347712 - Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - structural changes.

https://reviewboard.mozilla.org/r/121952/#review124086

f+, not sure how to do f+ through mozreview so right now it shows up at r+

::: tools/lint/eslint/eslint-plugin-mozilla/lib/configs/xpcshell-test.js:70
(Diff revision 1)
> +    "throws": false,
> +    "todo": false,
> +    "todo_check_false": false,
> +    "todo_check_true": false,
> +    // Firefox specific function.
> +    /* eslint max-len:off */

Should this be eslint-disable-next-line ?

::: tools/lint/eslint/eslint-plugin-mozilla/lib/index.js:5
(Diff revision 1)
> - * License, v. 2.0. If a copy of the MPL was not distributed with this
> - * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + * License, v. 2."error". If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2."error"/.

find and replace error

::: tools/lint/eslint/eslint-plugin-mozilla/lib/index.js:57
(Diff revision 1)
> -    "avoid-removeChild": 0,
> -    "avoid-nsISupportsString-preferences": 0,
> +    "avoid-removeChild": "error",
> +    "avoid-nsISupportsString-preferences": "error",

Thanks, this fixes bug 1339042.
Attachment #8849120 - Flags: review+
Comment on attachment 8849121 [details]
Bug 1347712 - Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - automatically update .eslintrc.js test config files for new config locations.

https://reviewboard.mozilla.org/r/121954/#review124088

f+, I don't know how to mark f+ through mozreview so this appears as r+ for now.

::: browser/components/extensions/test/browser/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
>  module.exports = {  // eslint-disable-line no-undef

Why is no-undef getting disabled here? Does that get inherited by extends?

::: browser/components/extensions/test/xpcshell/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
>  module.exports = {  // eslint-disable-line no-undef

Why is no-undef getting disabled here? Does that get inherited by extends?

::: browser/components/migration/tests/unit/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
>  module.exports = { // eslint-disable-line no-undef

Why is no-undef getting disabled here? Does that get inherited by extends?

::: browser/extensions/formautofill/test/browser/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
>  module.exports = { // eslint-disable-line no-undef

Why is no-undef getting disabled here? Does that get inherited by extends?

::: browser/extensions/formautofill/test/unit/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
>  module.exports = { // eslint-disable-line no-undef

Why is no-undef getting disabled here? Does that get inherited by extends?

::: browser/tools/mozscreenshots/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
>  module.exports = { // eslint-disable-line no-undef

Why is no-undef getting disabled here? Does that get inherited by extends?

::: devtools/.eslintrc.xpcshell.js:8
(Diff revision 1)
>    "extends": [
> -    "../testing/xpcshell/xpcshell.eslintrc.js"
> +    "plugin:mozilla/xpcshell-test"
>    ],
>    "rules": {
>      // Allow non-camelcase so that run_test doesn't produce a warning.
>      "camelcase": 0,

While you're here can you change these from 0 to "off" ?

::: toolkit/components/extensions/test/browser/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
>  module.exports = { // eslint-disable-line no-undef

Why is no-undef getting disabled here? Does that get inherited by extends?

::: toolkit/components/extensions/test/browser/.eslintrc.js:17
(Diff revision 1)
>      "ExtensionTestUtils": false,
>      "XPCOMUtils": true,
>    },
>  
>    "rules": {
>      "no-shadow": 0,

Can you change 0 to "off" while you're here?

::: toolkit/components/extensions/test/xpcshell/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
>  module.exports = { // eslint-disable-line no-undef

Why is no-undef getting disabled here? Does that get inherited by extends?

::: toolkit/modules/subprocess/test/xpcshell/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
>  module.exports = { // eslint-disable-line no-undef

Why is no-undef getting disabled here? Does that get inherited by extends?

::: toolkit/mozapps/extensions/test/xpinstall/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
>  module.exports = { // eslint-disable-line no-undef

Why is no-undef getting disabled here? Does that get inherited by extends?
Attachment #8849121 - Flags: review+
Comment on attachment 8849120 [details]
Bug 1347712 - Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - structural changes.

https://reviewboard.mozilla.org/r/121952/#review124094

::: tools/lint/eslint/eslint-plugin-mozilla/lib/index.js:57
(Diff revision 1)
> -    "avoid-removeChild": 0,
> -    "avoid-nsISupportsString-preferences": 0,
> +    "avoid-removeChild": "error",
> +    "avoid-nsISupportsString-preferences": "error",

0 should be "off". Are you sure that you wanted to change these from "off" to "error"? That's not clear to me. https://bugzilla.mozilla.org/show_bug.cgi?id=1316096#c0

I guess you want these to default to on instead of off?
Attachment #8849120 - Flags: review+
Attachment #8849120 - Flags: feedback?(jaws)
Attachment #8849120 - Flags: feedback+
Attachment #8849121 - Flags: review+
Attachment #8849121 - Flags: feedback?(jaws)
Attachment #8849121 - Flags: feedback+
Blocks: 1339042
Comment on attachment 8849121 [details]
Bug 1347712 - Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - automatically update .eslintrc.js test config files for new config locations.

https://reviewboard.mozilla.org/r/121954/#review124088

> Why is no-undef getting disabled here? Does that get inherited by extends?

This was previously for when we ran eslint on the .eslintrc.js files. Not sure what we want to do now.
Attachment #8849120 - Flags: review?(jaws)
Attachment #8849121 - Flags: review?(jaws)
Comment on attachment 8849120 [details]
Bug 1347712 - Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - structural changes.

https://reviewboard.mozilla.org/r/121952/#review124472
Attachment #8849120 - Flags: review?(jaws) → review+
Comment on attachment 8849121 [details]
Bug 1347712 - Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - automatically update .eslintrc.js test config files for new config locations.

https://reviewboard.mozilla.org/r/121954/#review124476
Attachment #8849121 - Flags: review?(jaws) → review+
Comment on attachment 8849570 [details]
Bug 1347712 - Move toolkit/.eslintrc.js rules into a 'recommended' set within eslint-plugin-mozilla.

https://reviewboard.mozilla.org/r/122350/#review124478
Attachment #8849570 - Flags: review?(jaws) → review+
Comment on attachment 8849571 [details]
Bug 1347712 - Add the tree-wide rules and config to the recommended eslint-plugin-mozilla config, to make it easier for outside projects.

https://reviewboard.mozilla.org/r/122352/#review124480
Attachment #8849571 - Flags: review?(jaws) → review+
(In reply to Mark Banner (:standard8) from comment #14)
> Comment on attachment 8849121 [details]
> Bug 1347712 - Change the testing configurations into ESLint configurations
> within eslint-plugin-mozilla - automatically update .eslintrc.js test config
> files for new config locations.
> 
> https://reviewboard.mozilla.org/r/121954/#review124088
> 
> > Why is no-undef getting disabled here? Does that get inherited by extends?
> 
> This was previously for when we ran eslint on the .eslintrc.js files. Not
> sure what we want to do now.

These seemed sprinkled throughout but not used in all cases. So in the specific case, I think we should remove them for consistency sake. I don't think we should push too hard to lint the configuration files, as its a much smaller codebase (now even smaller with these patches).
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6bf90d2bed35
Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - structural changes. r=jaws
https://hg.mozilla.org/integration/autoland/rev/1ee2cb31c1d1
Change the testing configurations into ESLint configurations within eslint-plugin-mozilla - automatically update .eslintrc.js test config files for new config locations. r=jaws
https://hg.mozilla.org/integration/autoland/rev/e52ee73071ad
Move toolkit/.eslintrc.js rules into a 'recommended' set within eslint-plugin-mozilla. r=jaws
https://hg.mozilla.org/integration/autoland/rev/6f536e25896d
Add the tree-wide rules and config to the recommended eslint-plugin-mozilla config, to make it easier for outside projects. r=jaws
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.