Enable the ESLint no-tabs rule across mozilla-central

RESOLVED FIXED in Firefox 56

Status

Testing
Lint
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: standard8, Assigned: rajk, Mentored)

Tracking

({good-first-bug})

Version 3
mozilla56
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [lang=js], URL)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 months ago
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
(Assignee)

Comment 1

5 months ago
I will work on this.
(Reporter)

Updated

5 months ago
Assignee: nobody → rajesh.kathiriya507
Comment hidden (mozreview-request)
(Reporter)

Comment 3

5 months ago
mozreview-review
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 */
Attachment #8883338 - Flags: review?(standard8)
Comment hidden (mozreview-request)
(Reporter)

Comment 5

5 months ago
mozreview-review
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
Attachment #8883338 - Flags: review?(standard8) → review+

Comment 6

5 months ago
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)
(Reporter)

Comment 7

5 months ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 10

4 months ago
Rajesh, it looks like something went wrong and there's no two commits, can you check the rebase and what you pushed please?
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8884021 - Attachment is obsolete: true
Attachment #8884021 - Flags: review?(standard8)

Comment 12

4 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe520e82e966
Enabled the ESLint no-tabs rule across mozilla-central r=standard8
(Reporter)

Comment 13

4 months ago
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)

Comment 14

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe520e82e966
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.