De-duplicate already ESLint recommended rules in security/manager/.eslintrc.js

RESOLVED FIXED in Firefox 55

Status

RESOLVED FIXED
2 years ago
20 days ago

People

(Reporter: standard8, Assigned: rajk, 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 security/manager/.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 security/manager/.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
can i take this one?
(Reporter)

Updated

2 years ago
Assignee: nobody → rajesh.kathiriya507
(Reporter)

Comment 2

2 years ago
I've just realised there's extra cleanup that can be done as well:

- If the .eslintrc.js file lists a rule as 'warn' and recommended.js lists it as 'error', then drop the rule in the .eslintrc.js file.

This will promote the rule to "error" which is better as it'll get fixed if it is an error, where as warnings tend to get ignored.
Comment hidden (mozreview-request)
(Reporter)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8860611 [details]
Bug 1354515 - Removed duplicate ESLint rules in security/manager

https://reviewboard.mozilla.org/r/132610/#review135524

Thank you for the patch. I think you removed a bit too much, but otherwise it generally looks good.

::: security/manager/.eslintrc.js
(Diff revision 1)
>    "rules": {
>      // Enforce return statements in callbacks of array methods.
>      "array-callback-return": "error",
>  
> -    // Braces only needed for multi-line arrow function blocks
> -    "arrow-body-style": ["error", "as-needed"],

This should stay (recommend.js has it, but it is commented out).

::: security/manager/.eslintrc.js
(Diff revision 1)
>  
> -    // Braces only needed for multi-line arrow function blocks
> -    "arrow-body-style": ["error", "as-needed"],
> -
> -    // Commas at the end of the line not the start
> -    "comma-style": "error",

Ditto - this should stay as well.

::: security/manager/.eslintrc.js
(Diff revision 1)
> -
>      // Verify calls of super() in constructors.
>      "constructor-super": "error",
>  
> -    // Require braces around blocks that start a new line
> -    "curly": ["error", "multi-line"],

This one should also stay.

::: security/manager/.eslintrc.js:21
(Diff revision 1)
>  
> -    // Always require a trailing EOL
> -    "eol-last": "error",
> -
>      // No spaces between function name and parentheses.
>      "func-call-spacing": ["error", "never"],

It turns out that "never" is the default, so we can remove this as well.

::: security/manager/.eslintrc.js
(Diff revision 1)
> -
>      // No spaces between function name and parentheses.
>      "func-call-spacing": ["error", "never"],
>  
> -    // Require function* name()
> -    "generator-star-spacing": ["error", {"before": false, "after": true}],

This one also needs to stay, please go through & check the rest that are commented out in recommended.js.

::: security/manager/.eslintrc.js:123
(Diff revision 1)
>  
>      // No space padding in parentheses
>      "space-in-parens": ["error", "never"],
>  
>      // Require spaces around operators
>      "space-infix-ops": "error",

recommend.js actually has the default args for "space-infix-ops", so this can be removed.

::: security/manager/.eslintrc.js:142
(Diff revision 1)
>  
>      // Disallow Yoda conditions.
>      "yoda": ["error", "never"],
>    },
>  
>    "globals": {

We can remove the complete globals section as well.
Attachment #8860611 - Flags: review?(standard8)
Comment hidden (mozreview-request)
(Reporter)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8860611 [details]
Bug 1354515 - Removed duplicate ESLint rules in security/manager

https://reviewboard.mozilla.org/r/132610/#review135798

Thank you for the updates. That's a lot better. r=Standard8.
Attachment #8860611 - Flags: review?(standard8) → review+

Comment 7

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9965cf60d2b7
Removed duplicate ESLint rules in security/manager r=standard8

Comment 8

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

Updated

11 months 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.