1378767 - Enable ESLint for extensions/pref/

Status: RESOLVED FIXED

(Core :: AutoConfig (Mission Control Desktop), enhancement)

Description

[Mark Banner (:standard8)]

As part of rolling out ESLint across the tree, we should enable it for the extensions/pref directory.

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

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

Here's some approximate steps:

- In .eslintignore, replace the existing `extensions/**` line with the following:

extensions/cookie/**
extensions/spellcheck/**
extensions/universalchardet/**

(this will exclude the directories we don't want to enable just yet).

- Create a copy of https://dxr.mozilla.org/mozilla-central/source/browser/components/places/tests/unit/.eslintrc.js into the extensions/prefs/autoconfig/test/unit/ directory.

(this is an environment specification that informs ESLint about our test setup)

- Run eslint:

./mach eslint --fix extensions

This should fix most/all of the issues.

- For the remaining issues, you'll need to fix them by hand.

For autoconfig.js, it is fine to specify 'pref' as a global with a line at the start of the file:

/* global pref */

- Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok.
- Create a commit and push it to mozreview:

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

If you're not sure of how to fix something, please look at the rules documentation http://eslint.org/docs/rules/ or just ask here.

Comment 1

[Hemant Singh Patwal]

Can I work on this?

Comment 2

[Mark Banner (:standard8)]

Sure.

Comment 3

[Hemant Singh Patwal]

Attachment: Bug 1378767 - Enable ESLint for extensions/pref/

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

Comment 4

[Hemant Singh Patwal]

Comment on attachment 8884028 [details]
Bug 1378767 - Enable ESLint for extensions/pref/

https://reviewboard.mozilla.org/r/154980/#review160026

::: extensions/pref/autoconfig/src/prefcalls.js:152
(Diff revision 1)
>                                    .createInstance(nsILDAPSyncQuery);
>          // default to LDAP v3
>          if (!gVersion)
>            gVersion = Components.interfaces.nsILDAPConnection.VERSION3
> -        // user supplied method
> + // user supplied method
>          processLDAPValues(ldapquery.getQueryResults(url, gVersion));

`no-undef` here.I am not sure what to import to fix it .

::: extensions/pref/autoconfig/test/unit/test_autoconfig_nonascii.js:17
(Diff revision 1)
>                 getService(Ci.nsIProperties);
> -  let obsvc = Cc['@mozilla.org/observer-service;1'].
> +  let obsvc = Cc["@mozilla.org/observer-service;1"].
>                getService(Ci.nsIObserverService);
>    let ps = Cc["@mozilla.org/preferences-service;1"].
>             getService(Ci.nsIPrefService);
>    let defaultPrefs = ps.getDefaultBranch(null);

showing `no-unused-vars`.
I am not sure to turn this off here.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8884028 [details]
Bug 1378767 - Enable ESLint for extensions/pref/

https://reviewboard.mozilla.org/r/154980/#review160196

Looks good so far. Just the couple of comments & expected changes to fix up.

::: extensions/pref/autoconfig/src/prefcalls.js:151
(Diff revision 1)
>          var ldapquery = Components.classes[LDAPSyncQueryContractID]
>                                    .createInstance(nsILDAPSyncQuery);
>          // default to LDAP v3
>          if (!gVersion)
>            gVersion = Components.interfaces.nsILDAPConnection.VERSION3
> -        // user supplied method
> + // user supplied method

nit: please fix the indentation of the comment.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8884028 [details]
Bug 1378767 - Enable ESLint for extensions/pref/

https://reviewboard.mozilla.org/r/154980/#review160026

> `no-undef` here.I am not sure what to import to fix it .

Lets just put this at the start of the file:

// If using LDAP autoconfig, we expect this to be defined by the user.
/* global processLDAPValues */

> showing `no-unused-vars`.
> I am not sure to turn this off here.

You can just remove the line - it is unused and we don't need the default branch here.

Comment 7

[Hemant Singh Patwal]

Comment on attachment 8884028 [details]
Bug 1378767 - Enable ESLint for extensions/pref/

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

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8884028 [details]
Bug 1378767 - Enable ESLint for extensions/pref/

https://reviewboard.mozilla.org/r/154980/#review160220

Great, thanks for the update. This looks good.

For future reference, if once you do an update, you can go into the reviews page and mark the outstanding issues as fixed, that would be useful (or mark them as fixed as you go through them!)

Comment 9

[Hemant Singh Patwal]

> For future reference, if once you do an update, you can go into the reviews
> page and mark the outstanding issues as fixed, that would be useful (or mark
> them as fixed as you go through them!)

Yeah! cool.

Comment 10

[Mark Banner (:standard8)]

(In reply to Hemant Singh Patwal [:hhhh1612] from comment #9)
> > For future reference, if once you do an update, you can go into the reviews
> > page and mark the outstanding issues as fixed, that would be useful (or mark
> > them as fixed as you go through them!)
> 
> Yeah! cool.

Can you go to https://reviewboard.mozilla.org/r/154980/#issue-summary and mark the two remaining as fixed please? Mozreview won't let me do so, and we can't push until they're closed.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d83718fd4da9
Enable ESLint for extensions/pref/ r=standard8

Comment 12

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

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