Closed
Bug 1309796
Opened 9 years ago
Closed 9 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•9 years ago
|
Flags: qe-verify?
Assignee | ||
Comment 1•9 years ago
|
||
It's an engineering only issue
Assignee: nobody → gasolin
Flags: qe-verify? → qe-verify-
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 2•9 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•9 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•9 years ago
|
||
So, this bug is about disallowing requiring chrome code, correct?
Honza
Updated•9 years ago
|
Flags: needinfo?(gasolin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•9 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•9 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•9 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•9 years ago
|
Priority: -- → P1
Comment 11•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Updated•9 years ago
|
Iteration: --- → 52.2 - Oct 17
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•