Closed
Bug 1150279
Opened 9 years ago
Closed 8 years ago
audit and turn on default eslint rules for Loop
Categories
(Hello (Loop) :: Client, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
backlog | tech-debt |
People
(Reporter: dmosedale, Unassigned)
References
Details
(Whiteboard: [tech-debt])
As part of bootstrapping ESLint, a lot of the default rules are turned off in browser/components/loop/.eslintrc. We should go through each of these rules, and either decide that we want to keep it off and add a comment as to why, or remove the line from the .eslintrc and fix the warnings that are uncovered. I suspect doing these all in one bug would result in unnecessary reviewer burden. I can imagine this work either being done in batches, or having a 1 bug per rule, or some combination of these things. I'd suggest that we leave this a little unformed, and whoever picks it up can spin off a bug with the approach that he/she prefers, and see how that goes.
Flags: firefox-backlog+
Updated•9 years ago
|
Assignee: nperriault → nobody
Rank: 35
Priority: -- → P3
Comment 1•9 years ago
|
||
I couldn't find the browser/components/loop/.eslintrc in http://lxr.mozilla.org/mozilla-central/source/browser/components/loop/ I was actually hacking on this locally and came up with the following (w/ eslint@1.3.1): ## /.eslintrc ``` extends: eslint:recommended plugins: - react ecmaFeatures: jsx: true env: browser: true mocha: true node: true globals: _: false Backbone: false chai: false loop: false React: false sinon: false rules: camelcase: 0 comma-spacing: 0 curly: 0 new-cap: 0 no-multi-spaces: 0 no-new: 0 no-trailing-spaces: 0 no-underscore-dangle: 0 no-use-before-define: [2, nofunc] no-wrap-func: 0 quotes: [1, double] semi-spacing: 0 space-infix-ops: 0 yoda: 0 ``` ## /.eslintignore ``` content/libs/l10n-gaia-*.js content/shared/js/.module-cache/ content/shared/libs/ test/shared/vendor/ ``` Currently I'm seeing "✖ 163 problems (128 errors, 35 warnings)"; https://gist.github.com/pdehaan/35dd71908dc013436819
Comment 2•9 years ago
|
||
Also, not sure if it's worth adding a "lint" script or something to the package.json to make linting a bit easier to run since you need to specify multiple file extensions. For example: ``` "scripts": { "lint": "eslint --ext=.{js,jsx,jsm} .", "test": "make test", "start": "make runserver" }, ``` Or add a "lint" task to the Makefile and have the npm script call "make lint", if we prefer.
Reporter | ||
Comment 3•9 years ago
|
||
(In reply to Peter deHaan [:pdehaan] from comment #1) > I couldn't find the browser/components/loop/.eslintrc in > http://lxr.mozilla.org/mozilla-central/source/browser/components/loop/ lxr and mxr don't seem to show dot-files. Happily, dxr does: https://dxr.mozilla.org/mozilla-central/source/browser/components/loop/ > > I was actually hacking on this locally and came up with the following (w/ > eslint@1.3.1): > > [...] Cool! > Currently I'm seeing "✖ 163 problems (128 errors, 35 warnings)"; > https://gist.github.com/pdehaan/35dd71908dc013436819 Right now, "mach eslint" run from browser/components/loop with our default files shows 0 errors and warnings. I think there's something to be said for adding a Makefile target or something to npm, though.
Comment 4•9 years ago
|
||
(In reply to Peter deHaan [:pdehaan] from comment #1) > I couldn't find the browser/components/loop/.eslintrc in > http://lxr.mozilla.org/mozilla-central/source/browser/components/loop/ It is there, you either need to use dxr: https://dxr.mozilla.org/mozilla-central/source/browser/components/loop/ or reference it direct: http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/.eslintrc REAME.txt tells you how to set it up and run it: http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/README.txt There's also "./mach eslint browser/components/loop" that we need to add to that readme at some stage (another project made that work).
Comment 5•9 years ago
|
||
(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #3) > I think there's something to be said for adding a Makefile target or > something to npm, though. Our Makefile/npm setup is specific to standalone. We have ./mach eslint which is as good as a Makefile target when you work with m-c, so I don't think we should add anything else.
Reporter | ||
Comment 6•9 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #5) > > Our Makefile/npm setup is specific to standalone. We have ./mach eslint > which is as good as a Makefile target when you work with m-c, so I don't > think we should add anything else. I can buy that for now, but I suspect if we restructure our code hierarchy at all for go-faster, we'll want to rethink that...
Reporter | ||
Comment 8•8 years ago
|
||
Ianb: looks like there's one dependent bug (jsdoc validation) that we want to do still. That said, we're probably in a good enough place to close this bug. Standard8, do you agree?
Flags: needinfo?(dmose) → needinfo?(standard8)
Comment 9•8 years ago
|
||
We'll track that separately. Lets close this.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(standard8)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•