Closed Bug 1374670 Opened 2 years ago Closed 2 years ago

Enable the ESLint no-tabs rule across mozilla-central


(Firefox Build System :: Lint and Formatting, enhancement)

3 Branch
Not set


(firefox56 fixed)

Tracking Status
firefox56 --- fixed


(Reporter: standard8, Assigned: rajk, Mentored)




(Whiteboard: [lang=js])


(1 file, 1 obsolete file)

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:

Also, details of the rule:

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/):

- Fix the instances it raises, and check that the indentation of the lines looks OK.
- Create a commit and push it to mozreview:
I will work on this.
Assignee: nobody → rajesh.kathiriya507
Comment on attachment 8883338 [details]
Bug 1374670 - Enabled the ESLint no-tabs rule across mozilla-central

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("payload"))[0]);
> +      callback(JSON.parse("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 */
Attachment #8883338 - Flags: review?(standard8)
Comment on attachment 8883338 [details]
Bug 1374670 - Enabled the ESLint no-tabs rule across mozilla-central

Looks great. r=Standard8
Attachment #8883338 - Flags: review?(standard8) → review+
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)
Rajesh, it looks like something conflicted with you between your last update and this one. Can you rebase and push again? Thank you!
Flags: needinfo?(rajesh.kathiriya507)
Rajesh, it looks like something went wrong and there's no two commits, can you check the rebase and what you pushed please?
Attachment #8884021 - Attachment is obsolete: true
Attachment #8884021 - Flags: review?(standard8)
Pushed by
Enabled the ESLint no-tabs rule across mozilla-central r=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.
Flags: needinfo?(rajesh.kathiriya507)
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Testing → Firefox Build System
Keywords: good-first-bug
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.