1354520 - De-duplicate already ESLint recommended rules in toolkit/components/reader/.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/reader/.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/reader/.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

[Jeremy Lam]

Hi Mark, I would like to work on this.

Comment 2

[Mark Banner (:standard8)]

Sure, please do.

Comment 3

[Jeremy Lam]

Hi Mark, I have started working on this but .eslintrc.js for me doesn't contain a globals and env section. It only has one section for rules. I am working through them with the specifications you mentioned but just want to make sure I am not missing anything.

Comment 4

[Mark Banner (:standard8)]

(In reply to Jeremy Lam from comment #3)
> Hi Mark, I have started working on this but .eslintrc.js for me doesn't
> contain a globals and env section. It only has one section for rules. I am
> working through them with the specifications you mentioned but just want to
> make sure I am not missing anything.

That's fine. Just ignore the references to the globals/env sections - I used the same wording as a template and forgot to check about those sections.

Comment 5

[Mark Banner (:standard8)]

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 6

[Hemant Singh Patwal]

Hi 

It's been a while since there is no activity.I can go through it if assigned to me.

Comment 7

[Mark Banner (:standard8)]

I've not heard from Jeremy, so assigning to Hemant.

Jeremy: if you do want an easy bug, let me know and I can find one for you.

Comment 8

[Hemant Singh Patwal]

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

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

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8863747 [details]
Bug 1354520 - De-duplicate already ESLint recommended rules in toolkit/components/reader/.eslintrc.js

https://reviewboard.mozilla.org/r/135526/#review138908

Looks great, just one minor point to address.

::: toolkit/components/reader/.eslintrc.js
(Diff revision 1)
> -
> -    // No expressions where a statement is expected
> -    // "no-unused-expressions": "error",
> -
> -    // No declaring variables that are never used
> -    "no-unused-vars": ["error", {"vars": "all", "args": "none"}],

Please keep the no-unused-vars here - it is slightly more strict than the recommended configuration (it applies to globals+locals rather than just locals)

Comment 10

[Hemant Singh Patwal]

Comment on attachment 8863747 [details]
Bug 1354520 - De-duplicate already ESLint recommended rules in toolkit/components/reader/.eslintrc.js

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

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8863747 [details]
Bug 1354520 - De-duplicate already ESLint recommended rules in toolkit/components/reader/.eslintrc.js

https://reviewboard.mozilla.org/r/135526/#review139254

Great, thank you for the update! r=Standard8.

Comment 12

[Pulsebot]

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

Comment 13

[Hemant Singh Patwal]

(In reply to Mark Banner (:standard8) from comment #11)
> Comment on attachment 8863747 [details]
> Bug 1354520 - De-duplicate already ESLint recommended rules in
> toolkit/components/reader/.eslintrc.js
> 
> https://reviewboard.mozilla.org/r/135526/#review139254
> 
> Great, thank you for the update! r=Standard8.

Please feel free to assign me bugs in future.:)

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46af4ac75c82