1228628 - Make eslint apply to the entire tree

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dave Townsend [:mossop]]

Currently mach eslint only check in devtools, loop and android. We want to extend that to be available to the whole tree.

Comment 1

[Dave Townsend [:mossop]]

Attachment: Create a top-level eslintignore file

Comment 2

[:Gijs (he/him)]

Comment on attachment 8693063 [details] [diff] [review]
Create a top-level eslintignore file

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

Forwarding to mconley because I need to run out, but looks mostly OK to me.

One thing to note is that this will auto-enable new directories. Because negations don't work I don't think there's a way around this, but it would be good to announce this publicly because it will catch people out if/when we start enforcing this.

::: .eslintignore
@@ +1,5 @@
> +# Always ignore node_modules.
> +**/node_modules/**/*.*
> +
> +# Exclude expected objdirs.
> +obj/**

This should probably be more generic, like obj*/** or something? By default the objdir will be obj-@CONFIG_GUESS@ which won't match this.

Comment 3

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 8693063 [details] [diff] [review]
> Create a top-level eslintignore file
> 
> Review of attachment 8693063 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Forwarding to mconley because I need to run out, but looks mostly OK to me.
> 
> One thing to note is that this will auto-enable new directories. Because
> negations don't work I don't think there's a way around this, but it would
> be good to announce this publicly because it will catch people out if/when
> we start enforcing this.

I don't think we can avoid this as you day but it could be a good thing if once we're enforcing any new code people write has to pass the linting rules or be rejected from the tree ;)

> ::: .eslintignore
> @@ +1,5 @@
> > +# Always ignore node_modules.
> > +**/node_modules/**/*.*
> > +
> > +# Exclude expected objdirs.
> > +obj/**
> 
> This should probably be more generic, like obj*/** or something? By default
> the objdir will be obj-@CONFIG_GUESS@ which won't match this.

Good catch

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8693063 [details] [diff] [review]
Create a top-level eslintignore file

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

Stoked for robot overlords.

::: .eslintignore
@@ +58,5 @@
> +# browser/ exclusions
> +browser/app/**
> +browser/base/**
> +browser/branding/**
> +browser/components/about/**

As mentioned in IRC, I don't think this will ignore things directly under browser/components, like nsBrowserGlue.js.

Same probably goes for any JS files in other subfolders where we're doing sub-subfolder ignores.

@@ +103,5 @@
> +# Libs we don't need to check
> +browser/components/loop/content/libs
> +browser/components/loop/content/shared/libs
> +browser/components/loop/standalone/content/libs
> +browser/components/loop/standalone/node_modules

Is this not redundant with the **/node_modules/**/*.* rule up top?

@@ +108,5 @@
> +# Libs we don't need to check
> +browser/components/loop/test/shared/vendor
> +# Coverage files
> +browser/components/loop/test/coverage
> +browser/components/loop/test/node_modules

Also redundant?

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/85bf0c3e44fd

Comment 6

[Mark Banner (:standard8)]

Attachment: Add a minimal .eslintrc configuration for browser and start linting a few browser files with basic rules.

This adds a minimal .eslintrc globally, and then enables one rule for browser: eol-last.

As a side effect of turning on es6, it also requires that "yield"s are in functions marked as generators (which our parser has allows not to happen atm).

We can turn on more rules/directories as we go.

Mossop gave me r+ over irc.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/343223ce6b34

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8693116 [details] [diff] [review]
Add a minimal .eslintrc configuration for browser and start linting a few browser files with basic rules.

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

::: browser/experiments/Experiments.jsm
@@ +143,5 @@
>  // OS.File.read.
>  // Returns a Promise resolved with the json payload or rejected with
>  // OS.File.Error or JSON.parse() errors.
>  function loadJSONAsync(file, options) {
> +  return Task.spawn(function*() {

This change is breaking xpcshell tests on fx-team. |Task.Result| isn't supported with star functions. You need to change that throw to a return.

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/40981d27ace0 since I needed to see xpcshell without this failing.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/58e64709ae1d

Comment 11

[Mark Banner (:standard8)]

(In reply to Kris Maglione [:kmag] from comment #8)
> This change is breaking xpcshell tests on fx-team. |Task.Result| isn't
> supported with star functions. You need to change that throw to a return.

Oops, sorry about that, I've fixed it and re-pushed.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/85bf0c3e44fd

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/58e64709ae1d