1501932 - Enable ESLint for modules/

Status: RESOLVED FIXED

(Core :: General, enhancement, P2)

Description

[Mark Banner (:standard8)]

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

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

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

Please note: We want to end up with two separate commits. One with the automatic changes, the second with the manual changes. It helps with reviewing if these appear as a commit series in phabricator.

Here's some approximate steps:

- In .eslintignore, remove the `modules/**` line.

- Run eslint:

./mach eslint --fix modules

This should fix most/all of the issues.

- Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok.

- Create a commit of the work so far, e.g.

$ hg commit -m "Bug nnn - Enable ESLint for modules/ (automatic changes)" modules/

- For the remaining issues, you'll need to fix them by hand. Most of those should be reasonably easy to understand, but there's more information on some specific bits (especially no-undef) here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ESLint#Common_issues_and_how_to_solve_them
http://eslint.org/docs/rules/

(you can just run `./mach eslint modules` to check you've fixed them).

- Create a second commit of the manual changes, e.g.

$ hg commit -m "Bug nnn - Enable ESLint for modules/ (manual changes)"

- Post the two commits via phabricator: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
When you post, please request initial review from myself (`standard8`), I'll then redirect to the owners of the code if the changes look good.

Comment 1

[kndarppatel]

Hi!
I'd like to pick this up.

Comment 2

[Mark Banner (:standard8)]

Hi, please do. Just follow the instructions, if you've got any questions feel free to ask.

Comment 3

[Ya'ar Hever]

Attachment: Bug 1501932 - Enable ESLint for modules/ (automatic changes)

Comment 4

[Ya'ar Hever]

Attachment: Bug 1501932 - Enable ESLint errors for modules/ (manual fixes)

Depends on D14526

Comment 5

[Ya'ar Hever]

Hello!

I wanted to pick up this bug before kndarppatel, but decided to leave it when he asked to pick it up. Since 2 months have passed, I thought I'd try again. I didn't know that submitting it to phabricator (I used moz-phab) would make it immediately appear here. I also don't know how to request a specific reviewer. I saw that Mark Banner (standard8) appears as a subscriber to this bug.

I made the commits as requested. In my first commit with the automatic fixes, I committed by mistake the temporary file of my first manual fix: modules/libpref/test/unit_ipc/test_user_default_prefs.js.new -- but I removed it in the second commit.

I'm used to git and am quite new to mercurial, so I couldn't figure out how to edit my commits after the fact (as I would with: git rebase --interactive).

Most of the changes I made to fix the ESLint errors had to do with the new syntax of the XPCOM services.

I'd be happy to get feedback! :)

Comment 6

[Mark Banner (:standard8)]

Hi Ya'ar, normally I'd comment first before taking over, but seeing as it has been 2 months, and kndarppatel hasn't given updates, I think that's reasonable.

Regarding editing existing commits, you should be able to use `hg histedit` it works in a similar way to `git rebase -i`. So if you can remove the temporary file completely, that would be good.

Note that you can also get a git repository for mozilla-central via https://github.com/glandium/git-cinnabar

Comment 7

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #6)
> Regarding editing existing commits, you should be able to use `hg histedit`
> it works in a similar way to `git rebase -i`. So if you can remove the
> temporary file completely, that would be good.

I should have mentioned: you probably need to be on a reasonably recent version of Mercurial if you're not already, and there's also `hg commit --amend` available.

Comment 8

[Ya'ar Hever]

Thank you very much for your response and for the review.

I wanted to comment before taking over. I thought I'd be able to first submit to phabricator, then comment and only then request a review. Sorry, I'm new.

I'll try to figure out `hg histedit` and submit again.

Comment 9

[Ya'ar Hever]

Attachment: Bug 1501932 - Enable ESLint for modules/ (automatic changes). r?njn!,aklotz!

Comment 10

[Ya'ar Hever]

Attachment: Bug 1501932 - Enable ESLint for modules/ (manual changes). r?njn!,aklotz!

Depends on D14645

Comment 11

[Ya'ar Hever]

I had to re-clone mozilla-central... `hg histedit` complained that there were merges in the history, so it couldn't edit it.

I made the changes that you requested and created 2 new commits with the same commit messages.

Comment 12

[Mark Banner (:standard8)]

(In reply to Ya'ar Hever from comment #11)
> I had to re-clone mozilla-central... `hg histedit` complained that there
> were merges in the history, so it couldn't edit it.
> 
> I made the changes that you requested and created 2 new commits with the
> same commit messages.

Thank you for the updates. Unfortunately I can't say why it didn't let you update. If you have to do this in future, it would be better to look at the old commits, and copy across the "Differential Revision" line - that tells moz-phab to update the existing commits (even if its in a completely different copy of the repo), rather than creating new ones.

Please could you visit the original two commits (D14526 and D14527) - links at the top of the bugzilla report - and go to the bottom of the page, and in the "Add Action" box, select "Abandon Revision" and then "Submit". If you do that for both, it'll mark them as abandoned and obsolete.

Comment 13

[Mark Banner (:standard8)]

Unfortunately I've not heard back from Ya'ar, so I'll steal this and help push the good work over the finishing line.

Comment 14

[Mark Banner (:standard8)]

Attachment: Bug 1501932 - Enable ESLint for modules/ (manual changes). r?njn!,aklotz!

Depends on D14645

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc657cbebdae
Enable ESLint for modules/ (automatic changes). r=njn,aklotz
https://hg.mozilla.org/integration/autoland/rev/f473b0c9b01a
Enable ESLint for modules/ (manual changes). r=aklotz,njn

Comment 16

[Mark Banner (:standard8)]

Assigning back to Ya'ar as this is now landing and they did most of the work here and I'd like to recognise that.

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/dc657cbebdae
https://hg.mozilla.org/mozilla-central/rev/f473b0c9b01a