Closed
Bug 1354517
Opened 7 years ago
Closed 7 years ago
De-duplicate already ESLint recommended rules in toolkit/components/extensions/.eslintrc.js
Categories
(Developer Infrastructure :: Lint and Formatting, enhancement)
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: standard8, Assigned: tanayseven, Mentored)
References
Details
(Whiteboard: [lang=js])
Attachments
(1 file)
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)
Assignee | ||
Comment 1•7 years ago
|
||
Hello, I'd like to fix this bug. Please assign it to me.
Reporter | ||
Comment 2•7 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•7 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•7 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•7 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•7 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•7 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)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 9•7 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•7 years ago
|
Flags: needinfo?(standard8)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 11•7 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•7 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•7 years ago
|
||
Thanks a lot, Mark. I'm really happy that my first patch has been merged into the main repository.
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4cdcb380564b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Product: Testing → Firefox Build System
Updated•5 years ago
|
Keywords: good-first-bug
Version: Version 3 → 3 Branch
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•