1430615 - Remove duplicated ESLint rules from devtools/.eslintrc.js

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[Mark Banner (:standard8)]

As part of tidying up ESLint rules for devtools, we want to de-duplicate the rules - remove rules from devtools/.eslintrc.js that are already defined the same in tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js.

There's also some now obsolete definitions for rules that can also be removed (they were obsoleted in ESLint 1.x, and we're now on 4.x).

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1430615 - Remove ESLint rules duplicated in devtools/.eslintrc.js where they are already defined in recommended.js.

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

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8942704 [details]
Bug 1430615 - Remove ESLint rules duplicated in devtools/.eslintrc.js where they are already defined in recommended.js.

https://reviewboard.mozilla.org/r/212972/#review218620

Thanks for the cleanup! Some comments and mostly a question about requiring the parent config file.

Would it be fine to add to devtools' eslintrc.js the following property:

  "extends": "../.eslintrc.js",
  
I think a lot of devtools developers have their editor setup to do eslint verification on the fly.
Without this, the editor has no way to know about the rules coming from the top-level eslintrc.

::: devtools/.eslintrc.js:113
(Diff revision 1)
>      "key-spacing": ["error", {"beforeColon": false, "afterColon": true}],
> -    // Enforces unix style line breaks.
> -    "linebreak-style": ["error", "unix"],
>      // Don't enforce the maximum depth that blocks can be nested. The complexity
>      // rule is a better rule to check this.
>      "max-depth": "off",

this one is also in recommended js

::: devtools/.eslintrc.js:165
(Diff revision 1)
> -    // Disallow if as the only statement in an else block.
> -    "no-lonely-if": "error",
>      // Allow mixing regular variable and require declarations (not a node env).
>      "no-mixed-requires": "off",
>      // Disallow mixed spaces and tabs for indentation.
>      "no-mixed-spaces-and-tabs": "error",

This is very similar to the current value in recommended.js which is:

  "no-mixed-spaces-and-tabs": ["error", "smart-tabs"],
  
I'm not sure how smart-tabs is relevant knowing that we also have no-tabs: "error"?

::: devtools/.eslintrc.js:208
(Diff revision 1)
>      // Still, making this a warning can help people avoid being confused.
>      "no-shadow": "error",
> -    // Disallow shadowing of names such as arguments.
> -    "no-shadow-restricted-names": "error",
>      // Deprecated, will be removed in 1.0.
>      "no-space-before-semi": "off",

deprecated, should be removed

::: devtools/.eslintrc.js:226
(Diff revision 1)
> -    "no-unreachable": "error",
>      // Disallow global and local variables that aren't used, but allow unused
>      // function arguments.
>      "no-unused-vars": ["error", {"vars": "all", "args": "none"}],
>      // Disallow flow control that escapes from "finally".
>      "no-unsafe-finally": "error",

this one is also in recommended js

::: devtools/.eslintrc.js:252
(Diff revision 1)
>      "semi-spacing": ["error", {"before": false, "after": true}],
>      // Don't require to sort variables within the same declaration block.
>      // Anyway, one-var is disabled.
>      "sort-vars": "off",
>      // Deprecated, will be removed in 1.0.
>      "space-after-function-name": "off",

deprecated, should be removed

::: devtools/.eslintrc.js:254
(Diff revision 1)
> -    // Require a space around all keywords.
> -    "keyword-spacing": "error",
> -    // Require a space before the start brace of a block.
> -    "space-before-blocks": ["error", "always"],
>      // Deprecated, will be removed in 1.0.
>      "space-before-function-parentheses": "off",

deprecated, should be removed

::: devtools/.eslintrc.js:268
(Diff revision 1)
> -    "space-infix-ops": ["error", {"int32Hint": true}],
>      // Require spaces before/after unary operators (words on by default,
>      // nonwords off by default).
>      "space-unary-ops": ["error", { "words": true, "nonwords": false }],
>      // Deprecated, will be removed in 1.0.
>      "space-unary-word-ops": "off",

deprecated, should be removed

::: devtools/.eslintrc.js:314
(Diff revision 1)
>      // disallow usage of __iterator__ property
>      "no-iterator": "off",
>      // disallow labels that share a name with a variable
>      "no-label-var": "off",
>      // disallow use of labeled statements
>      "no-labels": "error",

this one is in recommended.js

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8942704 [details]
Bug 1430615 - Remove ESLint rules duplicated in devtools/.eslintrc.js where they are already defined in recommended.js.

https://reviewboard.mozilla.org/r/212972/#review218620

I looked at doing this, and it messes up the glob based configurations that we have at the top level. So this isn't a good idea.

If we get specific issues with setups, we'll look at those as they come up.

> This is very similar to the current value in recommended.js which is:
> 
>   "no-mixed-spaces-and-tabs": ["error", "smart-tabs"],
>   
> I'm not sure how smart-tabs is relevant knowing that we also have no-tabs: "error"?

Oh, thanks for pointing that out. I've filed bug 1431129 for fixing this in recommended.js. I'll drop no-mixed-spaces-and-tabs from here, since the no-tabs rule seems to be covering things anyway.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8942704 [details]
Bug 1430615 - Remove ESLint rules duplicated in devtools/.eslintrc.js where they are already defined in recommended.js.

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

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8942704 [details]
Bug 1430615 - Remove ESLint rules duplicated in devtools/.eslintrc.js where they are already defined in recommended.js.

https://reviewboard.mozilla.org/r/212972/#review219386

Looks good to me, thanks!

As discussed on IRC, moving rules to the top level eslintrc means that devs that use Sublime (and maybe other editors?) with /devtools as the root folder of their project can no longer rely on the editor to check those rules.
Will send an email to dev-developer-tools and mention it during our next meeting to broadcast the info.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4451acb05678
Remove ESLint rules duplicated in devtools/.eslintrc.js where they are already defined in recommended.js. r=jdescottes

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4451acb05678