Enable ESLint for modules/

RESOLVED FIXED in Firefox 67

Status

()

P2
normal
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: standard8, Assigned: yaarhever, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

Trunk
mozilla67
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 months ago
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.
Priority: -- → P2

Comment 1

5 months ago
Hi!
I'd like to pick this up.
(Reporter)

Comment 2

5 months ago
Hi, please do. Just follow the instructions, if you've got any questions feel free to ask.
Assignee: nobody → kndarppatel
(Assignee)

Comment 4

3 months ago
Depends on D14526
(Assignee)

Comment 5

3 months ago
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! :)
(Reporter)

Comment 6

3 months ago
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
Assignee: kndarppatel → yaarhever
Attachment #9031334 - Attachment description: Bug 1501932 - enable ESLint for modules/ (automatic changes) → Bug 1501932 - Enable ESLint for modules/ (automatic changes)
Attachment #9031335 - Attachment description: Bug 1501932 - manually fix ESLint errors for modules/ → Bug 1501932 - Enable ESLint errors for modules/ (manual fixes)
(Reporter)

Comment 7

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

Comment 8

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

Comment 11

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

Comment 12

3 months ago
(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.
Flags: needinfo?(yaarhever)
(Reporter)

Comment 13

a month ago

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

Assignee: yaarhever → standard8
Flags: needinfo?(yaarhever)
Attachment #9031334 - Attachment is obsolete: true
Attachment #9031335 - Attachment is obsolete: true
Attachment #9031623 - Attachment description: Bug 1501932 - Enable ESLint for modules/ (automatic changes) → Bug 1501932 - Enable ESLint for modules/ (automatic changes). r?njn!,r?aklotz!
Attachment #9044271 - Attachment is obsolete: true
Attachment #9031623 - Attachment description: Bug 1501932 - Enable ESLint for modules/ (automatic changes). r?njn!,r?aklotz! → Bug 1501932 - Enable ESLint for modules/ (automatic changes). r?njn!,aklotz!
Attachment #9031625 - Attachment description: Bug 1501932 - Enable ESLint errors for modules/ (manual fixes) → Bug 1501932 - Enable ESLint for modules/ (manual changes). r?njn!,aklotz!

Comment 15

a month ago
Pushed by mbanner@mozilla.com:
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
(Reporter)

Comment 16

a month ago

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.

Assignee: standard8 → yaarhever

Comment 17

a month ago
bugherder
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
status-firefox66: --- → ?
status-firefox65: affected → wontfix
status-firefox66: ? → wontfix
You need to log in before you can comment on or make changes to this bug.