1245649 - Turn on more eslint rules in toolkit and browser

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Comment 1

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1245649: Merge browser and toolkit eslint rule settings. r=felipe

Review commit: https://reviewboard.mozilla.org/r/33487/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33487/

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 8715508 [details]
MozReview Request: Bug 1245649: Merge browser and toolkit eslint rule settings. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33487/diff/1-2/

Comment 3

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1245649: Turn on no-trailing-spaces. r=Gijs

Review commit: https://reviewboard.mozilla.org/r/33587/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33587/

Comment 4

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1245649: Turn on linebreak-style. r=Gijs

Review commit: https://reviewboard.mozilla.org/r/33589/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33589/

Comment 5

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1245649: Turn on no-extra-semi. r=markh

Review commit: https://reviewboard.mozilla.org/r/33591/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33591/

Comment 6

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1245649: Turn on valid-typeof, no-invalid-regexp, no-empty-pattern and no-empty-character-class. r=felipe

Review commit: https://reviewboard.mozilla.org/r/33593/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33593/

Comment 7

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1245649: Turn on no-irregular-whitespace and no-mixed-spaces-and-tabs.

Review commit: https://reviewboard.mozilla.org/r/33595/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33595/

Comment 8

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1245649: Turn on no-irregular-whitespace and no-mixed-spaces-and-tabs. r=jaws

Review commit: https://reviewboard.mozilla.org/r/33597/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33597/

Comment 9

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1245649: Turn on use-isnan, no-unexpected-multiline, no-octal and no-self-compare. r=MattN

Review commit: https://reviewboard.mozilla.org/r/33599/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33599/

Comment 10

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1245649: Enable no-negated-in-lhs, no-native-reassign, no-func-assign and no-labels. r=MattN

Review commit: https://reviewboard.mozilla.org/r/33601/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33601/

Comment 11

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1245649: Enable no-nested-ternary.

Review commit: https://reviewboard.mozilla.org/r/33603/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33603/

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8715508 [details]
MozReview Request: Bug 1245649: Merge browser and toolkit eslint rule settings. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33487/diff/2-3/

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8715661 [details]
MozReview Request: Bug 1245649: Turn on no-trailing-spaces. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33587/diff/1-2/

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8715662 [details]
MozReview Request: Bug 1245649: Turn on linebreak-style. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33589/diff/1-2/

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8715663 [details]
MozReview Request: Bug 1245649: Turn on no-extra-semi. r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33591/diff/1-2/

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 8715664 [details]
MozReview Request: Bug 1245649: Turn on valid-typeof, no-invalid-regexp, no-empty-pattern and no-empty-character-class. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33593/diff/1-2/

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 8715665 [details]
MozReview Request: Bug 1245649: Turn on no-irregular-whitespace and no-mixed-spaces-and-tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33595/diff/1-2/

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 8715666 [details]
MozReview Request: Bug 1245649: Turn on no-irregular-whitespace and no-mixed-spaces-and-tabs. r=jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33597/diff/1-2/

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 8715667 [details]
MozReview Request: Bug 1245649: Turn on use-isnan, no-unexpected-multiline, no-octal and no-self-compare. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33599/diff/1-2/

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8715668 [details]
MozReview Request: Bug 1245649: Enable no-negated-in-lhs, no-native-reassign, no-func-assign and no-labels. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33601/diff/1-2/

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 8715669 [details]
MozReview Request: Bug 1245649: Enable no-nested-ternary.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33603/diff/1-2/

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8715667 [details]
MozReview Request: Bug 1245649: Turn on use-isnan, no-unexpected-multiline, no-octal and no-self-compare. r=MattN

https://reviewboard.mozilla.org/r/33599/#review30407

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8715665 [details]
MozReview Request: Bug 1245649: Turn on no-irregular-whitespace and no-mixed-spaces-and-tabs.

https://reviewboard.mozilla.org/r/33595/#review30411

Mossop said that `hg diff -w` reports no changes, and I trust eslint to detect any erroneous whitespace changes. rs=me

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8715666 [details]
MozReview Request: Bug 1245649: Turn on no-irregular-whitespace and no-mixed-spaces-and-tabs. r=jaws

https://reviewboard.mozilla.org/r/33597/#review30413

LGTM

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 8715668 [details]
MozReview Request: Bug 1245649: Enable no-negated-in-lhs, no-native-reassign, no-func-assign and no-labels. r=MattN

https://reviewboard.mozilla.org/r/33601/#review30415

Comment 26

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8715663 [details]
MozReview Request: Bug 1245649: Turn on no-extra-semi. r=markh

https://reviewboard.mozilla.org/r/33591/#review30417

Awesome!

Comment 27

[Mark Hammond [:markh] [:mhammond]]

https://reviewboard.mozilla.org/r/33591/#review30419

Comment 28

[:Gijs (he/him)]

Comment on attachment 8715661 [details]
MozReview Request: Bug 1245649: Turn on no-trailing-spaces. r=Gijs

https://reviewboard.mozilla.org/r/33587/#review30409

::: toolkit/components/console/content/consoleBindings.xml:393
(Diff revision 2)
>            

Might as well remove this while we're here... I'm assuming this isn't being seen because the transforms we do on XBL don't leave in the empty lines between methods?

::: toolkit/components/places/Bookmarks.jsm:696
(Diff revision 2)
> -          `INSERT OR IGNORE INTO moz_places (url, rev_host, hidden, frecency, guid) 
> +          `INSERT OR IGNORE INTO moz_places (url, rev_host, hidden, frecency, guid)

Note that this changes the string we're using here (and elsewhere in this file).

I believe that this is OK because there's still a boatload of whitespace between this and the VALUES clause later in the SQL statement, but it will lose us identity with earlier slow sql statement things. Likely fine, just something to watch out for when replacing whitespace in files as there could conceivably be less innocuous cases.

In fact, there's an argument to be made that the eslint thing should not complain about such trailing spacing inside template strings.

::: toolkit/content/widgets/radio.xml:195
(Diff revision 2)
>        

Nit: kill this too

::: toolkit/content/widgets/toolbar.xml:95
(Diff revision 2)
>    

and some more

::: toolkit/content/widgets/tree.xml:1323
(Diff revision 2)
>          ]]></body>        

and more

::: toolkit/content/widgets/tree.xml:1381
(Diff revision 2)
>          ]]></body>        

and more

::: toolkit/content/widgets/wizard.xml:171
(Diff revision 2)
>         

and in this binding

Comment 29

[:Gijs (he/him)]

Comment on attachment 8715662 [details]
MozReview Request: Bug 1245649: Turn on linebreak-style. r=Gijs

https://reviewboard.mozilla.org/r/33589/#review30421

Comment 30

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8715508 [details]
MozReview Request: Bug 1245649: Merge browser and toolkit eslint rule settings. r=felipe

https://reviewboard.mozilla.org/r/33487/#review30423

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8715664 [details]
MozReview Request: Bug 1245649: Turn on valid-typeof, no-invalid-regexp, no-empty-pattern and no-empty-character-class. r=felipe

https://reviewboard.mozilla.org/r/33593/#review30425

Comment 32

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/33587/#review30409

> Might as well remove this while we're here... I'm assuming this isn't being seen because the transforms we do on XBL don't leave in the empty lines between methods?

Yeah, eslint only sees the JS pieces of the XBL and even then we're fairly aggressive at stripping leading and trailing whitespace of that since it is often used to lining up XML tags so there are a few cases that eslint will miss trailing whitespace in those cases.

> Note that this changes the string we're using here (and elsewhere in this file).
> 
> I believe that this is OK because there's still a boatload of whitespace between this and the VALUES clause later in the SQL statement, but it will lose us identity with earlier slow sql statement things. Likely fine, just something to watch out for when replacing whitespace in files as there could conceivably be less innocuous cases.
> 
> In fact, there's an argument to be made that the eslint thing should not complain about such trailing spacing inside template strings.

Yeah I had seen the change and surmised like you that it wouldn't change the effect because there is still whitespace to the rest of the SQL. I hadn't considered the slow sql thing but I don't think it should matter.

Comment 33

[Dave Townsend [:mossop]]

Comment on attachment 8715508 [details]
MozReview Request: Bug 1245649: Merge browser and toolkit eslint rule settings. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33487/diff/3-4/

Comment 34

[Dave Townsend [:mossop]]

Comment on attachment 8715661 [details]
MozReview Request: Bug 1245649: Turn on no-trailing-spaces. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33587/diff/2-3/

Comment 35

[Dave Townsend [:mossop]]

Comment on attachment 8715662 [details]
MozReview Request: Bug 1245649: Turn on linebreak-style. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33589/diff/2-3/

Comment 36

[Dave Townsend [:mossop]]

Comment on attachment 8715663 [details]
MozReview Request: Bug 1245649: Turn on no-extra-semi. r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33591/diff/2-3/

Comment 37

[Dave Townsend [:mossop]]

Comment on attachment 8715664 [details]
MozReview Request: Bug 1245649: Turn on valid-typeof, no-invalid-regexp, no-empty-pattern and no-empty-character-class. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33593/diff/2-3/

Comment 38

[Dave Townsend [:mossop]]

Comment on attachment 8715666 [details]
MozReview Request: Bug 1245649: Turn on no-irregular-whitespace and no-mixed-spaces-and-tabs. r=jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33597/diff/2-3/

Comment 39

[Dave Townsend [:mossop]]

Comment on attachment 8715667 [details]
MozReview Request: Bug 1245649: Turn on use-isnan, no-unexpected-multiline, no-octal and no-self-compare. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33599/diff/2-3/

Comment 40

[Dave Townsend [:mossop]]

Comment on attachment 8715668 [details]
MozReview Request: Bug 1245649: Enable no-negated-in-lhs, no-native-reassign, no-func-assign and no-labels. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33601/diff/2-3/

Comment 41

[Dave Townsend [:mossop]]

Comment on attachment 8715669 [details]
MozReview Request: Bug 1245649: Enable no-nested-ternary.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33603/diff/2-3/

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/345ff6c8b0a2
https://hg.mozilla.org/integration/fx-team/rev/6c56f5dad3b2
https://hg.mozilla.org/integration/fx-team/rev/06982056b5a5
https://hg.mozilla.org/integration/fx-team/rev/cb217f7271c2
https://hg.mozilla.org/integration/fx-team/rev/8b6ee0350b2c
https://hg.mozilla.org/integration/fx-team/rev/f3018a3c2de5
https://hg.mozilla.org/integration/fx-team/rev/f1d82662fe93
https://hg.mozilla.org/integration/fx-team/rev/3c4c76215a0d

Comment 43

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/345ff6c8b0a2
https://hg.mozilla.org/mozilla-central/rev/6c56f5dad3b2
https://hg.mozilla.org/mozilla-central/rev/06982056b5a5
https://hg.mozilla.org/mozilla-central/rev/cb217f7271c2
https://hg.mozilla.org/mozilla-central/rev/8b6ee0350b2c
https://hg.mozilla.org/mozilla-central/rev/f3018a3c2de5
https://hg.mozilla.org/mozilla-central/rev/f1d82662fe93
https://hg.mozilla.org/mozilla-central/rev/3c4c76215a0d

Comment 44

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

Comment on attachment 8715669 [details]
MozReview Request: Bug 1245649: Enable no-nested-ternary.

https://reviewboard.mozilla.org/r/33603/#review30555

Thanks Mossop! Great job. I will not be sorry to see these things go.

::: browser/base/content/browser-gestureSupport.js:429
(Diff revision 3)
> -      let getFunc = "get" + (type == "boolean" ? "Bool" :
> -                             type == "number" ? "Int" : "Char") + "Pref";
> -      return gPrefService[getFunc](branch + aPref);
> +      let getFunc = "Char";
> +      if (type == "boolean")
> +        getFunc = "Bool";
> +      else if (type == "number")
> +        getFunc = "Int";

I'll assume at this point, we haven't turned on the rule for "always braces around one-liner conditionals". :)

::: browser/extensions/pocket/content/pktApi.jsm:545
(Diff revision 3)
> -                    return a < b ? -1 : a > b ? 1 : 0;
> +                    return a - b;

Mossop++ for grand simplification.

::: toolkit/.eslintrc:112
(Diff revision 3)
>      // Nested ternary statements are confusing

You got that right.

::: toolkit/components/places/PlacesBackups.jsm:176
(Diff revision 3)
> -      return aDate < bDate ? 1 : aDate > bDate ? -1 : 0;
> +      return bDate.getTime() - aDate.getTime();

Do we really need .getTime() here?

::: toolkit/components/places/PlacesBackups.jsm:218
(Diff revision 3)
> -        return aDate < bDate ? 1 : aDate > bDate ? -1 : 0;
> +        return bDate.getTime() - aDate.getTime();

Same a above - doesn't just taking the deltas between Date's work, or am I missing something?

::: toolkit/components/reader/Readability.js:750
(Diff revision 3)
> -          var scoreDivider = level === 0 ? 1 : level === 1 ? 2 : level * 3;
> +          var scoreDivider = level < 2 ? level + 1 : level * 3;

Another great simplification.

::: toolkit/modules/Geometry.jsm:265
(Diff revision 3)
> -    let offsetX = (this.left <= other.left ? other.left - this.left :
> -                  (this.right > other.right ? other.right - this.right : 0));
> -    let offsetY = (this.top <= other.top ? other.top - this.top :
> -                  (this.bottom > other.bottom ? other.bottom - this.bottom : 0));
> +    let offsetX = 0;
> +    if (this.left <= other.left)
> +      offsetX = other.left - this.left;
> +    else if (this.right > other.right)
> +      offsetX = other.right - this.right;
> +    let offsetY = 0;
> +    if (this.top <= other.top)
> +      offsetY = other.top - this.top;
> +    else if (this.bottom > other.bottom)
> +      offsetY = other.bottom - this.bottom;

Nit: Maybe just break up the two blocks for offsetX and offsetY with a newline above 270.

Comment 45

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/33603/#review30555

> I'll assume at this point, we haven't turned on the rule for "always braces around one-liner conditionals". :)

We haven't, that's going to be a fun one to turn on, you're welcome to try!

> Do we really need .getTime() here?

You're right, I didn't know Date objects worked like that.

Comment 46

[Pulsebot]

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

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9181d9e2a6e

Comment 48

[:Gijs (he/him)]

This changed readability to use `let`.

The canonical readability repo is on github, and we can't use `let` there for compat reasons.

I'll probably land a workaround there to continue to use |var| and still not redeclare anything, and then merge that back into m-c.

Should we just ignore these files or enable a very limited set of rules in them or something? What would be the best way to deal with this?

Comment 49

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch from comment #48)
> This changed readability to use `let`.
> 
> The canonical readability repo is on github, and we can't use `let` there
> for compat reasons.

Sorry, didn't realise that was external code.

> I'll probably land a workaround there to continue to use |var| and still not
> redeclare anything, and then merge that back into m-c.
> 
> Should we just ignore these files or enable a very limited set of rules in
> them or something? What would be the best way to deal with this?

My general preference is just to add any code that we import wholesale from elsewhere to the ignore list so we're not spending time trying to fix things for their choices. If an external project has eslint set up and uses an eslintignore for example it will get ignored when imported so we'd start to see failures.

Comment 50

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #49)
> (In reply to :Gijs Kruitbosch from comment #48)
> > This changed readability to use `let`.
> > 
> > The canonical readability repo is on github, and we can't use `let` there
> > for compat reasons.
> 
> Sorry, didn't realise that was external code.
> 
> > I'll probably land a workaround there to continue to use |var| and still not
> > redeclare anything, and then merge that back into m-c.
> > 
> > Should we just ignore these files or enable a very limited set of rules in
> > them or something? What would be the best way to deal with this?
> 
> My general preference is just to add any code that we import wholesale from
> elsewhere to the ignore list so we're not spending time trying to fix things
> for their choices. If an external project has eslint set up and uses an
> eslintignore for example it will get ignored when imported so we'd start to
> see failures.

I wrote an .eslintrc for readability on github (https://github.com/mozilla/readability/pull/270 ) and added es6-disabling comments at the beginning of the files in question, but adding the specific files to eslintignore sounds good, so I'll land that in a bit.

Comment 51

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9b6dc52f6012

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b6dc52f6012