1411368 - Fix various issues revealed when upgrading to ESLint 4

Status: RESOLVED FIXED

(Toolkit :: General, enhancement, P2)

Description

[Mark Banner (:standard8)]

In bug 1371293 I'm working towards upgrading to ESLint 4. Currently there are lots of errors due to no-multi-spaces and spaced-comment detecting more issues on the ESLint 4 versions.

I'd like to land a patch to fix these, even if we haven't got rules to prevent them being re-introduced, as it'll help split up the main landing for bug 1371293 and avoid potential bitrot as I work on getting the remaining issues fixed.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1411368 - Automatically fix no-multi-spaces issues raised when using ESLint 4.

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

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 8921582 [details]
Bug 1411368 - Automatically fix no-multi-spaces issues raised when using ESLint 4.

https://reviewboard.mozilla.org/r/192602/#review198232

Is it at all possible to split this diff up by rule that is causing the change?

::: accessible/tests/mochitest/elm/test_canvas.html:31
(Diff revision 1)
>        context.beginPath();
>        context.rect(kX, kY, kWidth, kHeight);
>        context.addHitRegion({control: element});
>  
>        var input = getAccessible("showA");
> -      var [cnvX, cnvY, /*cnvWidth*/, /*cnvHeight*/] = getBoundsForDOMElm(canv);
> +      var [cnvX, cnvY, /* cnvWidth*/, /* cnvHeight*/] = getBoundsForDOMElm(canv);

This looks ugly to me. I assume there is a rule requiring a space after the start of a block comment, is there one requiring a space before the end too?

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8921582 [details]
Bug 1411368 - Automatically fix no-multi-spaces issues raised when using ESLint 4.

https://reviewboard.mozilla.org/r/192602/#review198232

It is a little bit more work, but I'm doing that now.

> This looks ugly to me. I assume there is a rule requiring a space after the start of a block comment, is there one requiring a space before the end too?

It looks like spaced-comment has an option for "balanced". For now, I've fixed the issues in this patch manually, but I can file a follow-up to enable that as well (I suspect there might be more instances to fix across the tree).

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1411368 - Automatically fix spaced-comment issues raised when using ESLint 4.

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

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1411368 - Automatically fix indent-legacy issues raised when using ESLint 4.

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

Comment 6

[Mark Banner (:standard8)]

Attachment: Bug 1411368 - Automatically fix padded-blocks issues raised when using ESLint 4.

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

Comment 7

[Mark Banner (:standard8)]

Attachment: Bug 1411368 - Automatically fix space-unary-ops issues raised when using ESLint 4.

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

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8921582 [details]
Bug 1411368 - Automatically fix no-multi-spaces issues raised when using ESLint 4.

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

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 8921582 [details]
Bug 1411368 - Automatically fix no-multi-spaces issues raised when using ESLint 4.

https://reviewboard.mozilla.org/r/192602/#review198666

::: browser/components/nsBrowserGlue.js:689
(Diff revision 2)
> -      let buildDate = new Date(buildID.slice(0, 4),     // year
> +      let buildDate = new Date(buildID.slice(0, 4), // year
>                                 buildID.slice(4, 6) - 1, // months are zero-based.
> -                               buildID.slice(6, 8),     // day
> -                               buildID.slice(8, 10),    // hour
> -                               buildID.slice(10, 12),   // min
> -                               buildID.slice(12, 14))   // ms
> +                               buildID.slice(6, 8), // day
> +                               buildID.slice(8, 10), // hour
> +                               buildID.slice(10, 12), // min
> +                               buildID.slice(12, 14)) // ms

Seems like this alignment is actually useful, can we ignore the rule here.

::: devtools/server/actors/emulation/touch-simulator.js:267
(Diff revision 2)
> -      unwrapped.sendTouchEvent(name, clone([0]),       // event type, id
> -                               clone([evt.clientX]),   // x
> -                               clone([evt.clientY]),   // y
> +      unwrapped.sendTouchEvent(name, clone([0]), // event type, id
> +                               clone([evt.clientX]), // x
> +                               clone([evt.clientY]), // y
>                                 clone([1]), clone([1]), // rx, ry
>                                 clone([0]), clone([0]), // rotation, force
> -                               1);                     // count
> +                               1); // count

Same here.

::: services/sync/modules/util.js:72
(Diff revision 2)
> -        Services.appinfo.name + "/" + Services.appinfo.version +  // Product.
> -        " (" + hph.oscpu + ")" +                                  // (oscpu)
> -        " FxSync/" + WEAVE_VERSION + "." +                        // Sync.
> -        Services.appinfo.appBuildID + ".";                        // Build.
> +        Services.appinfo.name + "/" + Services.appinfo.version + // Product.
> +        " (" + hph.oscpu + ")" + // (oscpu)
> +        " FxSync/" + WEAVE_VERSION + "." + // Sync.
> +        Services.appinfo.appBuildID + "."; // Build.

And here.

::: toolkit/mozapps/update/tests/chrome/utils.js:87
(Diff revision 2)
> -const PAGEID_DUMMY            = "dummy";                 // Done
> -const PAGEID_CHECKING         = "checking";              // Done
> -const PAGEID_NO_UPDATES_FOUND = "noupdatesfound";        // Done
> -const PAGEID_MANUAL_UPDATE    = "manualUpdate";          // Done
> -const PAGEID_UNSUPPORTED      = "unsupported";           // Done
> -const PAGEID_FOUND_BASIC      = "updatesfoundbasic";     // Done
> -const PAGEID_DOWNLOADING      = "downloading";           // Done
> -const PAGEID_ERRORS           = "errors";                // Done
> -const PAGEID_ERROR_EXTRA      = "errorextra";            // Done
> -const PAGEID_ERROR_PATCHING   = "errorpatching";         // Done
> -const PAGEID_FINISHED         = "finished";              // Done
> -const PAGEID_FINISHED_BKGRD   = "finishedBackground";    // Done
> +const PAGEID_DUMMY            = "dummy"; // Done
> +const PAGEID_CHECKING         = "checking"; // Done
> +const PAGEID_NO_UPDATES_FOUND = "noupdatesfound"; // Done
> +const PAGEID_MANUAL_UPDATE    = "manualUpdate"; // Done
> +const PAGEID_UNSUPPORTED      = "unsupported"; // Done
> +const PAGEID_FOUND_BASIC      = "updatesfoundbasic"; // Done
> +const PAGEID_DOWNLOADING      = "downloading"; // Done
> +const PAGEID_ERRORS           = "errors"; // Done
> +const PAGEID_ERROR_EXTRA      = "errorextra"; // Done
> +const PAGEID_ERROR_PATCHING   = "errorpatching"; // Done
> +const PAGEID_FINISHED         = "finished"; // Done
> +const PAGEID_FINISHED_BKGRD   = "finishedBackground"; // Done

It looks like we could get rid of these comments? They don't seem to add much value.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8922290 [details]
Bug 1411368 - Automatically fix spaced-comment issues raised when using ESLint 4.

https://reviewboard.mozilla.org/r/193348/#review198670

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8922292 [details]
Bug 1411368 - Automatically fix padded-blocks issues raised when using ESLint 4.

https://reviewboard.mozilla.org/r/193352/#review198674

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8922293 [details]
Bug 1411368 - Automatically fix space-unary-ops issues raised when using ESLint 4.

https://reviewboard.mozilla.org/r/193354/#review198676

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8922291 [details]
Bug 1411368 - Automatically fix indent-legacy issues raised when using ESLint 4.

https://reviewboard.mozilla.org/r/193350/#review198680

Seems like all these parts are indented the way they are with good reason. Can we exclude the rules here?

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8921582 [details]
Bug 1411368 - Automatically fix no-multi-spaces issues raised when using ESLint 4.

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

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8922290 [details]
Bug 1411368 - Automatically fix spaced-comment issues raised when using ESLint 4.

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

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8922291 [details]
Bug 1411368 - Automatically fix indent-legacy issues raised when using ESLint 4.

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

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8922292 [details]
Bug 1411368 - Automatically fix padded-blocks issues raised when using ESLint 4.

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

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8922293 [details]
Bug 1411368 - Automatically fix space-unary-ops issues raised when using ESLint 4.

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

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 8922291 [details]
Bug 1411368 - Automatically fix indent-legacy issues raised when using ESLint 4.

https://reviewboard.mozilla.org/r/193350/#review199066

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f21099e6fb5
Automatically fix no-multi-spaces issues raised when using ESLint 4. r=mossop
https://hg.mozilla.org/integration/autoland/rev/2ac26fcd2aa0
Automatically fix spaced-comment issues raised when using ESLint 4. r=mossop
https://hg.mozilla.org/integration/autoland/rev/f3b05acf98e4
Automatically fix indent-legacy issues raised when using ESLint 4. r=mossop
https://hg.mozilla.org/integration/autoland/rev/bb1bc004011f
Automatically fix padded-blocks issues raised when using ESLint 4. r=mossop
https://hg.mozilla.org/integration/autoland/rev/22cbc112b655
Automatically fix space-unary-ops issues raised when using ESLint 4. r=mossop

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1f21099e6fb5
https://hg.mozilla.org/mozilla-central/rev/2ac26fcd2aa0
https://hg.mozilla.org/mozilla-central/rev/f3b05acf98e4
https://hg.mozilla.org/mozilla-central/rev/bb1bc004011f
https://hg.mozilla.org/mozilla-central/rev/22cbc112b655

Comment 22

[[github robot]]

Commit pushed to master at https://github.com/mozilla/normandy

https://github.com/mozilla/normandy/commit/cb3ebd69ea6b9b76673851d619cf659fe9cb2481
Bug 1411368 - Automatically fix no-multi-spaces issues raised when using ESLint 4. r=mossop