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

RESOLVED FIXED in Firefox 55

Status

RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: standard8, Assigned: tanayseven, Mentored)

Tracking

3 Branch
mozilla55

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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)
(Reporter)

Updated

2 years ago
Blocks: 1354521
(Assignee)

Comment 1

2 years ago
Hello, I'd like to fix this bug. Please assign it to me.
(Reporter)

Comment 2

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

Sure, done.
Assignee: nobody → tanayseven
(Assignee)

Comment 3

2 years ago
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?
(Assignee)

Comment 4

2 years ago
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?
(Reporter)

Comment 5

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

Comment 6

2 years ago
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?
(Assignee)

Comment 7

2 years ago
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?
Flags: needinfo?(standard8)
(Reporter)

Comment 9

2 years ago
mozreview-review
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).
Attachment #8858500 - Flags: review?(standard8)
(Reporter)

Updated

2 years ago
Flags: needinfo?(standard8)
Comment hidden (mozreview-request)
(Reporter)

Comment 11

2 years ago
mozreview-review
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.
Attachment #8858500 - Flags: review?(standard8) → review+

Comment 12

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4cdcb380564b
De-duplicate already ESLint recommended rules in toolkit/components/extensions/.eslintrc.js r=standard8
(Assignee)

Comment 13

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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4cdcb380564b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

a year ago
Product: Testing → Firefox Build System
Keywords: good-first-bug
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.