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)

x86
macOS
defect

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+
Assignee: nperriault → nobody
Rank: 35
Priority: -- → P3
Depends on: 1158112
Depends on: 1160145
Depends on: 1160487
Depends on: 1160489
Depends on: 1160496
Depends on: 1160498
Depends on: 1160501
Depends on: 1162646
Depends on: 1165266
Depends on: 1165281
Depends on: 1170757
Depends on: 1174945
Depends on: 1176778
Depends on: 1176780
Depends on: 1176933
Depends on: 1181239
Depends on: 1181825
Depends on: 1193311
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
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.
(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.
(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).
(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.
(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...
Depends on: 1206683
Depends on: 1212331
No longer depends on: 1212331
Depends on: 1232994
Is this done?
Flags: needinfo?(dmose)
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)
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.