Closed
Bug 1309796
Opened 8 years ago
Closed 8 years ago
add eslint support for netmonitor
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox52 fixed)
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: gasolin, Assigned: gasolin)
References
(Blocks 1 open bug)
Details
(Whiteboard: [netmonitor])
Attachments
(1 file)
netmonitor/test/ has eslint support, but netmonitor/ does not. Expect: Add .eslintrc and fix related lint error in netmonitor/ repo
Updated•8 years ago
|
Flags: qe-verify?
Assignee | ||
Comment 1•8 years ago
|
||
It's an engineering only issue
Assignee: nobody → gasolin
Flags: qe-verify? → qe-verify-
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
When I append: `var notUsedVar;` into netmonitor/panel.js, and run: `./mach eslint devtools/client/netmonitor` I am getting an error as expected: Z 12:5 error 'notUsedVar' is defined but never used. no-unused-vars (eslint) ✖ 1 problem (1 error, 0 warnings) What errors in `netmonitor/` dir do you see? Honza
Assignee | ||
Comment 3•8 years ago
|
||
As inspector/.eslintrc , we can identify some chrome privilege requires and comment them in source https://github.com/mozilla/gecko-dev/blob/7e0678438a65e310f2392ca79b77cba6ec08f7f4/devtools/client/inspector/.eslintrc
Comment 4•8 years ago
|
||
So, this bug is about disallowing requiring chrome code, correct? Honza
Updated•8 years ago
|
Flags: needinfo?(gasolin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
Just want netmonitor have same configurations as inspector. It's not actually disallowing require chrome code, but only fix warnings by addressing them with disable flags.
Flags: needinfo?(gasolin)
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8800568 [details] Bug 1309796 - add eslint support for netmonitor; https://reviewboard.mozilla.org/r/85484/#review84132 Looks good to me, and Eslint is green for me. R+, assuming my comment is resolved. Thanks! Honza ::: devtools/client/netmonitor/.eslintrc:10 (Diff revision 1) > + "rules": { > + // The inspector is being migrated to HTML and cleaned of > + // chrome-privileged code, so this rule disallows requiring chrome > + // code. Some files in the inspector disable this rule still. The > + // goal is to enable the rule globally on all files. > + "mozilla/reject-some-requires": [2, "^(chrome|chrome:.*|resource:.*|devtools/server/.*|.*\\.jsm|devtools/shared/platform/(chome|content)/.*)$"], Please update the comment, it's used for the Network panel now (not the Inspector).
Attachment #8800568 -
Flags: review?(odvarko) → review+
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b2f3dd7eec5d add eslint support for netmonitor; r=Honza
Keywords: checkin-needed
Updated•8 years ago
|
Priority: -- → P1
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b2f3dd7eec5d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Updated•8 years ago
|
Iteration: --- → 52.2 - Oct 17
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•