Closed Bug 1378767 Opened 2 years ago Closed 2 years ago

Enable ESLint for extensions/pref/

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: standard8, Assigned: hemantsingh1612, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file)

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.
Keywords: good-first-bug
Whiteboard: [lang=js]
Assignee: mozilla → nobody
Can I work on this?
Sure.
Assignee: nobody → hemantsingh1612
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 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.
Attachment #8884028 - Flags: review?(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 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!)
Attachment #8884028 - Flags: review?(standard8) → review+
> 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.
(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.
Flags: needinfo?(hemantsingh1612)
Flags: needinfo?(hemantsingh1612)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d83718fd4da9
Enable ESLint for extensions/pref/ r=standard8
https://hg.mozilla.org/mozilla-central/rev/d83718fd4da9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.