1374670 - Enable the ESLint no-tabs rule across mozilla-central

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Mark Banner (:standard8)]

We currently have no-tabs enabled for a few directories, but we should expand it to the whole of the repository.

I'm happy to mentor this bug. There's background on our eslint setups here:

https://developer.mozilla.org/docs/ESLint

Also, details of the rule: http://eslint.org/docs/rules/no-tabs

Here's some approximate steps:

- Add a no-tabs (error) line in recommended.js.
- Remove the no-tabs related lines in the existing .eslintrc.js configs (apart from devtools/):

https://dxr.mozilla.org/mozilla-central/search?q=no-tabs&redirect=false

- Fix the instances it raises, and check that the indentation of the lines looks OK.
- Create a commit and push it to mozreview:

http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

Comment 1

[Rajesh Kathiriya(:rajk)]

I will work on this.

Comment 2

[Rajesh Kathiriya(:rajk)]

Attachment: Bug 1374670 - Enabled the ESLint no-tabs rule across mozilla-central

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

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8883338 [details]
Bug 1374670 - Enabled the ESLint no-tabs rule across mozilla-central

https://reviewboard.mozilla.org/r/154230/#review159516

This is looking good. Applying it locally, I see that testing/talos/talos/pageloader/chrome/report.js hasn't been fixed - I know we recently enabled ESLint on the testing/talos directories, so maybe you need to update & rebase?

There's a few other bits that just need improving as well:

::: accessible/tests/mochitest/jsat/output.js:23
(Diff revision 1)
>   * scoped to the "root" element in markup.
>   */
>  function testContextOutput(expected, aAccOrElmOrID, aOldAccOrElmOrID, aGenerator) {
>    var accessible = getAccessible(aAccOrElmOrID);
>    var oldAccessible = aOldAccOrElmOrID !== null ?
> -        getAccessible(aOldAccOrElmOrID || 'root') : null;
> +  getAccessible(aOldAccOrElmOrID || 'root') : null;

nit: please add 2 more spaces of indent here (4 total).

::: browser/extensions/pocket/content/panels/js/messages.js:26
(Diff revision 1)
>   }
>  
>   function addMessageListener(panelId, messageId, callback) {
>     document.addEventListener(panelPrefixedMessageId(panelId, messageId), function(e) {
>  
> -                        callback(JSON.parse(e.target.getAttribute("payload"))[0]);
> +      callback(JSON.parse(e.target.getAttribute("payload"))[0]);

Somehow this seems to be indented by 5 spaces, can you make it 4 please? (and the couple of lines below)).

::: browser/extensions/pocket/content/panels/js/messages.js:48
(Diff revision 1)
>       panelId,
>       data: (payload || {})
>     };
>  
> -                // Create a callback to listen for a response
> +    // Create a callback to listen for a response
> -                if (callback) {
> +    if (callback) {

This section has also been indented slightly wrongly, can you rebase it on 2-space indent please?

::: services/sync/tests/unit/test_utils_passphrase.js:11
(Diff revision 1)
>    const hyphenated = "2-6ect2-thczm-599m2-ffqar-bicjq";
>    do_check_eq(Utils.normalizePassphrase(hyphenated), pp);
>  
>    _("Skip whitespace.");
>    do_check_eq("aaaaaaaaaaaaaaaaaaaaaaaaaa", Utils.normalizePassphrase("aaaaaaaaaaaaaaaaaaaaaaaaaa  "));
> -  do_check_eq("aaaaaaaaaaaaaaaaaaaaaaaaaa", Utils.normalizePassphrase("	 aaaaaaaaaaaaaaaaaaaaaaaaaa"));
> +  do_check_eq("aaaaaaaaaaaaaaaaaaaaaaaaaa", Utils.normalizePassphrase("  aaaaaaaaaaaaaaaaaaaaaaaaaa"));

Giving this is a test file looking at whitespace, I think it is meant to have the tabs here.

Can you undo the changes to this file, and then add a /* eslint no-tabs:"off" */ near the beginning please.

::: toolkit/components/passwordmgr/test/unit/test_getPasswordFields.js:82
(Diff revision 1)
>      skipEmptyFields: true,
>    },
>    {
>      description: "skipEmptyFields should also skip white-space only fields",
>      document: `<input id="pw-space" type=password value=" ">
> -               <input id="pw-tab" type=password value="	">
> +               <input id="pw-tab" type=password value=" ">

This test looks like it is meant to contain a tab. Please undo the change here and in-between line 80/81 add a /* eslint-disable-next-line no-tabs */

Comment 4

[Rajesh Kathiriya(:rajk)]

Comment on attachment 8883338 [details]
Bug 1374670 - Enabled the ESLint no-tabs rule across mozilla-central

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

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8883338 [details]
Bug 1374670 - Enabled the ESLint no-tabs rule across mozilla-central

https://reviewboard.mozilla.org/r/154230/#review159878

Looks great. r=Standard8

Comment 6

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 24ffda36e642 -d cc69d09c37c4: rebasing 405535:24ffda36e642 "Bug 1374670 - Enabled the ESLint no-tabs rule across mozilla-central r=standard8" (tip)
merging accessible/jsat/AccessFu.jsm
merging accessible/jsat/Utils.jsm
merging accessible/tests/mochitest/textattrs/test_general.html
merging browser/base/content/browser.js
merging toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
merging toolkit/modules/sessionstore/FormData.jsm
warning: conflicts while merging toolkit/modules/sessionstore/FormData.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 7

[Mark Banner (:standard8)]

Rajesh, it looks like something conflicted with you between your last update and this one. Can you rebase and push again? Thank you!

Comment 8

[Rajesh Kathiriya(:rajk)]

Attachment: Bug 1374670 - Enabled the ESLint no-tabs rule across mozilla-central

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

Comment 9

[Rajesh Kathiriya(:rajk)]

Comment on attachment 8883338 [details]
Bug 1374670 - Enabled the ESLint no-tabs rule across mozilla-central

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

Comment 10

[Mark Banner (:standard8)]

Rajesh, it looks like something went wrong and there's no two commits, can you check the rebase and what you pushed please?

Comment 11

[Rajesh Kathiriya(:rajk)]

Comment on attachment 8883338 [details]
Bug 1374670 - Enabled the ESLint no-tabs rule across mozilla-central

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe520e82e966
Enabled the ESLint no-tabs rule across mozilla-central r=standard8

Comment 13

[Mark Banner (:standard8)]

Great, thank you for the updates. Looks like it should land alright now :-)

It is a good rule for us to get enabled as well.

Comment 14

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

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