Enable ESLint for netwerk/dns/ and netwerk/protocol/

RESOLVED FIXED in Firefox 65

Status

()

P3
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: standard8, Assigned: mkohler, Mentored)

Tracking

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

Trunk
mozilla65
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [lang=js][necko-triaged])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 months ago
As part of rolling out ESLint across the tree, we should enable it for the netwerk/dns/ and netwerk/protocol/ directories.

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 `netwerk/dns/**` and `netwerk/protocol/**` lines.

- Run eslint:

./mach eslint --fix netwerk

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 netwerk/dns/ and netwerk/protocol/ (automatic changes)" netwerk/

- 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 netwerk` to check you've fixed them).

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

$ hg commit -m "Bug nnn - Enable ESLint for netwerk/dns/ and netwerk/protocol/ (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.
(Assignee)

Comment 1

5 months ago
I'm down to one error, will finish that tomorrow evening and push a review.
Assignee: nobody → me
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [lang=js] → [lang=js][necko-triaged]
(Assignee)

Comment 2

5 months ago
Bug 1501621 - Enable ESLint for netwerk/dns/ and netwerk/protocol/ (manual changes)
(Assignee)

Updated

5 months ago
Attachment #9020091 - Attachment is obsolete: true
Attachment #9021901 - Attachment description: Bug 1501621 - Enable ESLint for netwerk/dns/ and netwerk/protocol/ (manual changes) Depends on D10590 → Bug 1501621 - Enable ESLint for netwerk/dns/ and netwerk/protocol/ (manual changes)
Attachment #9020091 - Attachment is obsolete: false
Attachment #9020091 - Attachment is obsolete: true
Attachment #9021896 - Attachment description: Bug 1501621 - Enable ESLint for netwerk/dns/ and netwerk/protocol/ (automatic changes) → Bug 1501621 - Enable ESLint for netwerk/dns/ and netwerk/protocol/ (automatic changes) r=Standard8,dragana
Attachment #9021901 - Attachment description: Bug 1501621 - Enable ESLint for netwerk/dns/ and netwerk/protocol/ (manual changes) → Bug 1501621 - Enable ESLint for netwerk/dns/ and netwerk/protocol/ (manual changes) r=Standard8,dragana
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 6

5 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9f6807ef4a0
Enable ESLint for netwerk/dns/ and netwerk/protocol/ (automatic changes) r=dragana,Standard8
https://hg.mozilla.org/integration/autoland/rev/a140c44db41d
Enable ESLint for netwerk/dns/ and netwerk/protocol/ (manual changes) r=dragana,Standard8
Keywords: checkin-needed
(Reporter)

Comment 7

5 months ago
Michael: Just so you know, you don't need to re-title the attachments, Lando adds the r= for us if it isn't already specified (and makes sure it is correct - in this case it actually changed the order!).
(Reporter)

Comment 8

5 months ago
Thank you for working on this, great to get these enabled.

Comment 9

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e9f6807ef4a0
https://hg.mozilla.org/mozilla-central/rev/a140c44db41d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox65: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.