Make eslint apply to the entire tree

RESOLVED FIXED in Firefox 45

Status

Firefox Build System
General
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: mossop, Unassigned)

Tracking

unspecified
mozilla45

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Currently mach eslint only check in devtools, loop and android. We want to extend that to be available to the whole tree.
(Reporter)

Comment 1

3 years ago
Created attachment 8693063 [details] [diff] [review]
Create a top-level eslintignore file

Comment 2

3 years ago
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.
Attachment #8693063 - Flags: review?(mconley)

Updated

3 years ago
Attachment #8693063 - Flags: feedback+
(Reporter)

Comment 3

3 years ago
(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 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?
Attachment #8693063 - Flags: review?(mconley) → review+
(Reporter)

Updated

3 years ago
Whiteboard: keep-open
Created attachment 8693116 [details] [diff] [review]
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.
Attachment #8693116 - Flags: review+
(Reporter)

Updated

3 years ago
Whiteboard: keep-open
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.

Updated

3 years ago
Flags: needinfo?(standard8)
Backed out in https://hg.mozilla.org/integration/fx-team/rev/40981d27ace0 since I needed to see xpcshell without this failing.
(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.
Flags: needinfo?(standard8)

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/85bf0c3e44fd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Reporter)

Updated

3 years ago
Blocks: 1229856

Updated

4 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.