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

RESOLVED FIXED in Firefox 55

Status

RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: standard8, Assigned: hemantsingh1612, Mentored)

Tracking

({good-first-bug})

3 Branch
mozilla55
good-first-bug

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(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/narrate/.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/narrate/.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
I am interested in working on this bug .Can it be assigned to me?
(Reporter)

Comment 2

2 years ago
(In reply to Hemant Singh Patwal from comment #1)
> I am interested in working on this bug .Can it be assigned to me?

Sure.
Assignee: nobody → hemantsingh1612
(Assignee)

Comment 3

2 years ago
Well I am getting this log after running ./mach eslint

$ ./mach eslint
eslint-plugin-react v4.2.3 needs to be installed locally.
escope v^3.6.0 needs to be installed locally.
eslint-plugin-html v1.5.2 needs to be installed locally.
estraverse v^4.2.0 needs to be installed locally.
eslint-plugin-mozilla v0.2.36 needs to be installed locally.
sax v^1.2.2 needs to be installed locally.
eslint v3.18.0 needs to be installed locally.
eslint-plugin-spidermonkey-js v0.1.0 needs to be installed locally.
ini-parser v^0.0.2 needs to be installed locally.
espree v^3.4.0 needs to be installed locally.
Installing eslint for mach using "C:\Program Files\nodejs\npm.cmd install --loglevel=error"...
Installing eslint-plugin-mozilla using "C:\Program Files\nodejs\npm.cmd install c:\mozilla-source\mozilla-central\tools\lint\eslint\eslint-plugin-mozilla --loglevel=error"...
Installing eslint-plugin-spidermonkey-js using "C:\Program Files\nodejs\npm.cmd install c:\mozilla-source\mozilla-central\tools\lint\eslint\eslint-plugin-spidermonkey-js --loglevel=error"...

ESLint and approved plugins installed successfully!

NOTE: Your local eslint binary is at c:\mozilla-source\mozilla-central\node_modules\.bin\eslint

An error occurred running eslint. Please check the following error messages:

'node' is not recognized as an internal or external command,
operable program or batch file.
A failure occured in the eslint linter.
? 1 problem (0 errors, 0 warnings, 1 failure)
(Reporter)

Comment 4

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.
(Reporter)

Comment 5

2 years ago
Hi Hemant,

(In reply to Hemant Singh Patwal [:hhhh1612] from comment #3)
> NOTE: Your local eslint binary is at
> c:\mozilla-source\mozilla-central\node_modules\.bin\eslint
> 
> An error occurred running eslint. Please check the following error messages:
> 
> 'node' is not recognized as an internal or external command,
> operable program or batch file.
> A failure occured in the eslint linter.
> ? 1 problem (0 errors, 0 warnings, 1 failure)

Where is your node.exe or node.cmd located? Is it in C:\Program Files\nodejs\ ? If so, I'd have expected this to be picked up automatically:

https://dxr.mozilla.org/mozilla-central/rev/c697e756f738ce37abc56f31bfbc48f55625d617/tools/lint/eslint.lint#200
(Assignee)

Comment 6

2 years ago

(In reply to Mark Banner (:standard8) from comment #4)
> 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.

Yeah I got it :).(In reply to Mark Banner (:standard8) from comment #5)
> Hi Hemant,
> 
> (In reply to Hemant Singh Patwal [:hhhh1612] from comment #3)
> > NOTE: Your local eslint binary is at
> > c:\mozilla-source\mozilla-central\node_modules\.bin\eslint
> > 
> > An error occurred running eslint. Please check the following error messages:
> > 
> > 'node' is not recognized as an internal or external command,
> > operable program or batch file.
> > A failure occured in the eslint linter.
> > ? 1 problem (0 errors, 0 warnings, 1 failure)
> 
> Where is your node.exe or node.cmd located? Is it in C:\Program
> Files\nodejs\ ? If so, I'd have expected this to be picked up automatically:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> c697e756f738ce37abc56f31bfbc48f55625d617/tools/lint/eslint.lint#200

Yes it is in C:\Program Files\nodejs.
(Assignee)

Comment 7

2 years ago
But it's not being picked automatically.
(Assignee)

Comment 8

2 years ago
My node version is 6.10.2.
(Assignee)

Comment 9

2 years ago
Here's the .eslintrc.js after removing duplicates:

"use strict";

module.exports = {
  "globals": {
    "Components": true,
    "dump": true,
    "Iterator": true
  },

  "rules": {
    "mozilla/no-aArgs": "warn",
    "mozilla/reject-importGlobalProperties": "warn",
    "mozilla/var-only-at-top-level": "warn",
    "block-scoped-var": "error",
    "brace-style": ["warn", "1tbs", {"allowSingleLine": false}],
    "camelcase": "warn",
    "comma-dangle": "off",
    "comma-style": ["warn", "last"],
    "complexity": ["error", {"max": 20}],
    "curly": "error",
    "dot-location": ["warn", "property"],
    "dot-notation": "error",
    "generator-star-spacing": ["warn", "after"],
    "key-spacing": ["warn", {"beforeColon": false, "afterColon": true}],
    "max-len": ["warn", 80, 2, {"ignoreUrls": true}],
    "max-nested-callbacks": ["error", 3],
    "new-cap": ["error", {"capIsNew": false}],
    "no-control-regex": "error",
    "no-eval": "error",
    "no-extend-native": "error",
    "no-fallthrough": "error",
    "no-inline-comments": "warn",
    "no-mixed-spaces-and-tabs": "error",
    "no-multi-spaces": "warn",
    "no-multi-str": "warn",
    "no-multiple-empty-lines": ["warn", {"max": 1}],
    "no-return-assign": "error",
    "no-sequences": "error",    
    "no-spaced-func": "warn",
    "no-throw-literal": "error",
    "no-unneeded-ternary": "error",
    "no-unused-vars": "error",
    "padded-blocks": ["warn", "never"],
    "quotes": ["warn", "double", "avoid-escape"],
    "semi-spacing": ["warn", {"before": false, "after": true}],
    "space-before-blocks": ["warn", "always"],
    "space-unary-ops": ["warn", { "words": true, "nonwords": false }],
    "spaced-comment": ["warn", "always"],
    "strict": ["error", "global"],
    "yoda": "error"
  }
};
(Reporter)

Comment 10

2 years ago
Hi Hemant, could you either attach a patch file, or push a commit to mozreview please? For more information about mozreview see http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

There's also https://developer.mozilla.org/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F if you need it.

This will allow us to track the changes easily and set the appropriate flags. Thanks.
(Assignee)

Comment 12

2 years ago
Posted patch rb132728.patch (obsolete) — Splinter Review
Attachment #8860770 - Flags: review?(standard8)
(Reporter)

Comment 13

2 years ago
Comment on attachment 8860770 [details] [diff] [review]
rb132728.patch

Review of attachment 8860770 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start, though I think there's various improvements that can be made. If you don't use mozreview for the next patch, please make sure it has a commit header & message on the patch.

::: toolkit/components/narrate/.eslintrc.js
@@ +4,3 @@
>    "globals": {
>      "Components": true,
>      "dump": true,

Both the "Components" and "dump" lines can be removed.

@@ +14,2 @@
>      "block-scoped-var": "error",
>      "brace-style": ["warn", "1tbs", {"allowSingleLine": false}],

This line is the same as one in recommended.js, but that one is an error, so lets remove this line and hence promote this to an error.

@@ +24,1 @@
>      "key-spacing": ["warn", {"beforeColon": false, "afterColon": true}],

I think this line can be removed as well, hence to promote to an error.

@@ -38,5 @@
>      "max-len": ["warn", 80, 2, {"ignoreUrls": true}],
>      "max-nested-callbacks": ["error", 3],
>      "new-cap": ["error", {"capIsNew": false}],
> -    "new-parens": "error",
> -    "no-array-constructor": "error",

This one is commented out in recommended.js, so it should stay for now.

@@ -79,3 @@
>      "padded-blocks": ["warn", "never"],
>      "quotes": ["warn", "double", "avoid-escape"],
> -    "semi": ["warn", "always"],

This line is commented out in recommended.js, so should stay in this file.

@@ -82,4 @@
>      "semi-spacing": ["warn", {"before": false, "after": true}],
>      "space-before-blocks": ["warn", "always"],
> -    "space-before-function-paren": ["warn", "never"],
> -    "space-in-parens": ["warn", "never"],

space-in-parens is also comment out, so should stay in.

@@ -87,1 @@
>      "space-unary-ops": ["warn", { "words": true, "nonwords": false }],

I think space-unary-ops is close enough that we can remove it from here and default to the recommended.js definition.

@@ -87,2 @@
>      "space-unary-ops": ["warn", { "words": true, "nonwords": false }],
>      "spaced-comment": ["warn", "always"],

The default for spaced-comment is "always", so this can be removed & hence will become an error due to recommended.js definition.
Attachment #8860770 - Flags: review?(standard8)
(Assignee)

Comment 15

2 years ago
Posted patch rb133118.patch (obsolete) — Splinter Review
I have modified the file as mentioned.Please have a look.
Attachment #8861168 - Flags: review?(standard8)
(Reporter)

Updated

2 years ago
Attachment #8860770 - Attachment is obsolete: true
(Reporter)

Comment 16

2 years ago
Comment on attachment 8861168 [details] [diff] [review]
rb133118.patch

No need to attach a patch separately if it is already in mozreview. There's a link to mozreview on the bug page (or the console when you push the patch) where you can add reviewers if necessary. I'll do that now.
Attachment #8861168 - Attachment is obsolete: true
Attachment #8861168 - Flags: review?(standard8)
(Reporter)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8861167 [details]
Bug 1354518 - De-duplicate already recommended ESLint rules in toolkit/components/narrate/.eslintrc.js.

https://reviewboard.mozilla.org/r/133120/#review136242

Thank you for pushing this to Mozreview. This is looking good so far. There's a few more changes to make, but once you've done those, we should be good to go.

::: commit-message-1a106:1
(Diff revision 1)
> +bug 1354518 -removed duplicates

Please change the commit title to be:

Bug 1354518 - De-duplicate already recommended ESLint rules in toolkit/components/narrate/.eslintrc.js. r?Standard8


This is much more descriptive for people looking through the commit lists.

::: toolkit/components/narrate/.eslintrc.js
(Diff revision 1)
>      "curly": "error",
>      "dot-location": ["warn", "property"],
>      "dot-notation": "error",
> -    "eol-last": "error",
>      "generator-star-spacing": ["warn", "after"],
> -    "indent": ["warn", 2, {"SwitchCase": 1}],

`indent` should stay because it is commented out in recommended.js

::: toolkit/components/narrate/.eslintrc.js
(Diff revision 1)
> -    "no-nested-ternary": "error",
> -    "no-redeclare": "error",
>      "no-return-assign": "error",
> -    "no-self-compare": "error",
> +    "no-sequences": "error",    
> -    "no-sequences": "error",
> -    "no-shadow": "warn",

`no-shadow` also needs to stay for now.

::: toolkit/components/narrate/.eslintrc.js:45
(Diff revision 1)
> -    "no-with": "error",
>      "padded-blocks": ["warn", "never"],
>      "quotes": ["warn", "double", "avoid-escape"],
>      "semi": ["warn", "always"],
>      "semi-spacing": ["warn", {"before": false, "after": true}],
>      "space-before-blocks": ["warn", "always"],

The default is "always", so `space-before-blocks` can also be removed.
Comment hidden (mozreview-request)
(Reporter)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8861167 [details]
Bug 1354518 - De-duplicate already recommended ESLint rules in toolkit/components/narrate/.eslintrc.js.

https://reviewboard.mozilla.org/r/133120/#review136288

Great. Thank you for the updates. This looks good now. r=Standard8

Thank you for your work on this.

Normally you could add checkin-needed at this stage, but I'll push the patch for you. It'll go to one of our integration branches, and assuming there's no issues found in the automated tests, then it'll get merged to the main mozilla-central within about 24 hours at which point this bug will be automatically marked as fixed.
Attachment #8861167 - Flags: review?(standard8) → review+

Comment 20

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

Comment 21

2 years ago
(In reply to Mark Banner (:standard8) from comment #19)
> Comment on attachment 8861167 [details]
> Bug 1354518 - De-duplicate already recommended ESLint rules in
> toolkit/components/narrate/.eslintrc.js.
> 
> https://reviewboard.mozilla.org/r/133120/#review136288
> 
> Great. Thank you for the updates. This looks good now. r=Standard8
> 
> Thank you for your work on this.
> 
> Normally you could add checkin-needed at this stage, but I'll push the patch
> for you. It'll go to one of our integration branches, and assuming there's
> no issues found in the automated tests, then it'll get merged to the main
> mozilla-central within about 24 hours at which point this bug will be
> automatically marked as fixed.

Thanks Mark.

It was interesting to work with.Learned new things. :)

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9a735c14664f
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
Keywords: good-first-bug
You need to log in before you can comment on or make changes to this bug.