1354517 - De-duplicate already ESLint recommended rules in toolkit/components/extensions/.eslintrc.js

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Mark Banner (:standard8)]

We now have a recommend set of rules for ESLint in Mozilla, I'd like for us to tidy up duplicates, so that it is easier to see where modules/different areas are different.

This bug is to tidy up toolkit/components/extensions/.eslintrc.js

I'm happy to mentor this.

Recommended rules: https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js

What needs doing (all in the file toolkit/components/extensions/.eslintrc.js):

- Remove the extends section, as this is unnecessary (ESLint inherits rules through the directory tree).
- For the rules, globals and env sections, go through each one and compare to the recommended, then:
-- If it is the same (with the same parameters), remove the rule
-- If it is commented, remove it
-- If it is different, leave it in.
- Finally check ESLint still runs correctly (./mach eslint) and passes with no errors (warnings are ok if they are the same as previous).

Once that's done, attach a patch and request review from me (Standard8)

Comment 1

[Tanay Prabhu Desai]

Hello, I'd like to fix this bug. Please assign it to me.

Comment 2

[Mark Banner (:standard8)]

(In reply to Tanay Prabhu Desai from comment #1)
> Hello, I'd like to fix this bug. Please assign it to me.

Sure, done.

Comment 3

[Tanay Prabhu Desai]

I tried to do 
./mach bootstrap
and the result of that was:
https://pastebin.mozilla.org/9018806
I cannot even clone a new repository. Did something go wrong with the configuration of Mercurial when I first ran ./bootstrap.py?

Comment 4

[Tanay Prabhu Desai]

Sorry for the above comment. I have resolved it. I just ran ./mach eslint and the output was as follows:
https://pastebin.mozilla.org/9018883
Is this the correct output at this stage, where I have not modified any code?

Comment 5

[Mark Banner (:standard8)]

(In reply to Tanay Prabhu Desai from comment #4)
> Sorry for the above comment. I have resolved it. I just ran ./mach eslint
> and the output was as follows:
> https://pastebin.mozilla.org/9018883
> Is this the correct output at this stage, where I have not modified any code?

Yes, that's fine. The warnings can be ignored.

Comment 6

[Tanay Prabhu Desai]

Some things that are in toolkit/components/extensions/.eslintrc.js are commented out in tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js. Should I leave the recommended.js file as it is?

Comment 7

[Tanay Prabhu Desai]

The "env" section in toolkit/components/extensions/.eslintrc.js had just "browser": true, which is exactly present in the env of tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js. So, should I leave an empty "env": { } ? Or should I remove it completely?

Comment 8

[Tanay Prabhu Desai]

Attachment: Bug 1354517 - De-duplicate already ESLint recommended rules in toolkit/components/extensions/.eslintrc.js

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

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8858500 [details]
Bug 1354517 - De-duplicate already ESLint recommended rules in toolkit/components/extensions/.eslintrc.js

https://reviewboard.mozilla.org/r/130466/#review133320

Hi Tanay, thank you for the patch. This is looking good so far.

Removing all of the "env" section was the right thing to do.

There's a few extra bits of tidy up I think we can do - I noticed there were a few places where there's a warn that we can promote to an error, so I think it would be good to clean those up as well.

If you can update the patch, then post a new version, I think we'll be at the point I can grant review and get this landed.

Thanks.

::: toolkit/components/extensions/.eslintrc.js:8
(Diff revision 1)
> -  },
>  
>    "globals": {
>      "Cc": true,
>      "Ci": true,
>      "Components": true,

You can remove this `"Components": true,` line as well.

::: toolkit/components/extensions/.eslintrc.js:61
(Diff revision 1)
> -
> -    // Require spacing around =>
> -    "arrow-spacing": "error",
> -
>      // Always require spacing around a single line block
>      "block-spacing": "warn",

Please can you remove this line & its comment as well - block-spacing is currently passing, so lets go with making this into an error (which is what the recommended config currently has).

::: toolkit/components/extensions/.eslintrc.js:94
(Diff revision 1)
> -
> -    // No duplicate cases in switch statements
> -    "no-duplicate-case": "error",
> -
>      // If an if block ends with a return no need for an else block
>      // "no-else-return": "error",

Please drop these two commented out lines.

::: toolkit/components/extensions/.eslintrc.js:104
(Diff revision 1)
> -
> -    // No assiging to exception variable
> -    "no-ex-assign": "error",
> -
>      // No using !! where casting to boolean is already happening
>      "no-extra-boolean-cast": "warn",

Please can you drop the `no-extra-boolean-cast` as well (recommended.js has this as an error which is better).

::: toolkit/components/extensions/.eslintrc.js:107
(Diff revision 1)
> -
> -    // No odd whitespace characters
> -    "no-irregular-whitespace": "error",
> -
>      // No single if block inside an else block
>      "no-lonely-if": "warn",

Please can you drop the `no-lonely-if` as well (recommended.js has this as an error which is better).

::: toolkit/components/extensions/.eslintrc.js:320
(Diff revision 1)
>  
>      // Don't require quotes around object literal property names.
>      "quote-props": "off",
>  
>      // Double quotes should be used.
>      "quotes": ["warn", "double", {"avoidEscape": true, "allowTemplateLiterals": true}],

Please can you drop the `quotes` as well (recommended.js has this as an error which is better).

Comment 10

[Tanay Prabhu Desai]

Comment on attachment 8858500 [details]
Bug 1354517 - De-duplicate already ESLint recommended rules in toolkit/components/extensions/.eslintrc.js

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

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8858500 [details]
Bug 1354517 - De-duplicate already ESLint recommended rules in toolkit/components/extensions/.eslintrc.js

https://reviewboard.mozilla.org/r/130466/#review134290

Thank you for the updates Tanay, this looks great and is ready to land, hence r+.

I'll land this on our integration branch for you in a couple of minutes, the bug will get marked as fixed when it is merged into our main repository.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cdcb380564b
De-duplicate already ESLint recommended rules in toolkit/components/extensions/.eslintrc.js r=standard8

Comment 13

[Tanay Prabhu Desai]

Thanks a lot, Mark. I'm really happy that my first patch has been merged into the main repository.

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/4cdcb380564b