Closed
Bug 1317069
Opened 8 years ago
Closed 8 years ago
Fix eslint for new console output
Categories
(DevTools :: Console, defect, P3)
DevTools
Console
Tracking
(firefox53 fixed)
RESOLVED
FIXED
Firefox 53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: ntim, Assigned: nchevobbe)
References
Details
Attachments
(1 file)
No description provided.
Reporter | ||
Updated•8 years ago
|
Blocks: devtools-eslint
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → chevobbe.nicolas
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
Sorry the stubs have been modified to to some formatting (and some packets changes too). Mocha tests are green, TRY is running https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eef8d040a536f62514420a63e4078ccf0ed0f39 Brian if you could have a quick look at this before the end of the week, I'll greatly appreciate :) Thanks !
Reporter | ||
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8817463 [details] Bug 1317069 - Fix ESLint in new console frontend; https://reviewboard.mozilla.org/r/97718/#review98092 ::: .eslintignore:106 (Diff revision 1) > -!devtools/client/webconsole/jsterm.js > -!devtools/client/webconsole/console-commands.js > +devtools/client/webconsole/console-output.js > +devtools/client/webconsole/hudservice.js > +devtools/client/webconsole/utils.js > +devtools/client/webconsole/webconsole-connection-proxy.js > +devtools/client/webconsole/webconsole.js > +devtools/client/webconsole/new-console-output/test/fixtures/stubs/ Can we create ESLint friendly subs? (or at least make them as ESLint friendly as possible and disable some rules for stubs). It bothers me just having the folder listed here :) ::: devtools/client/webconsole/new-console-output/main.js:5 (Diff revision 1) > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > - /* global BrowserLoader */ > + /* global BrowserLoader, window */ I don't have a strong preference, but you could set the eslint environment to browser in the config file. ::: devtools/client/webconsole/new-console-output/test/chrome/head.js:3 (Diff revision 1) > /* Any copyright is dedicated to the Public Domain. > http://creativecommons.org/publicdomain/zero/1.0/ */ > +/* exported Assert, Task, browserRequire */ a search on dxr tells me that Assert isn't being used in other files: https://dxr.mozilla.org/mozilla-central/search?q=Assert+path%3Adevtools%2Fclient%2Fwebconsole%2F&redirect=false
Assignee | ||
Comment 4•8 years ago
|
||
Thanks Tim :) Do you mind if I put you as a reviewer for this, since you already looked at it ?
> Can we create ESLint friendly subs? (or at least make them as ESLint friendly as possible and disable some rules for stubs). It bothers me just having the folder listed here :)
The stubs are generated, so I don't know if it makes sense to make them eslint-compliant. I'll check how difficult it can be and see.
Reporter | ||
Comment 5•8 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #4) > Thanks Tim :) Do you mind if I put you as a reviewer for this, since you > already looked at it ? Sure, go ahead :) > > Can we create ESLint friendly subs? (or at least make them as ESLint friendly as possible and disable some rules for stubs). It bothers me just having the folder listed here :) > > The stubs are generated, so I don't know if it makes sense to make them > eslint-compliant. I'll check how difficult it can be and see. We could do some small tweaks like using 2 space indentation or fixing the missing semis. If it introduces some unnecessary complications to fix a rule (such as max-len), you can just disable it for the file (/* eslint-disable max-len */ or using a eslint config file).
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8817463 [details] Bug 1317069 - Fix ESLint in new console frontend; https://reviewboard.mozilla.org/r/97718/#review98120 ::: devtools/client/webconsole/new-console-output/main.js:6 (Diff revision 2) > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > /* global BrowserLoader */ > + /* exported XPCOMUtils */ DXR tells me that XPCOMUtils isn't used anywhere except browser_webconsole_observer_notifications.js. The instance in the test should be defined by the test infrastructure, not by this file, so you should be able to safely remove the import in this file. If that's not the case, I'd rather have it defined within browser_webconsole_observer_notifications.js rather than globally. ::: devtools/client/webconsole/new-console-output/test/chrome/head.js:3 (Diff revision 2) > /* Any copyright is dedicated to the Public Domain. > http://creativecommons.org/publicdomain/zero/1.0/ */ > +/* exported Assert, Task, browserRequire */ nit: remove Assert from the exported comment as well ::: devtools/client/webconsole/new-console-output/test/components/network-event-message.test.js:62 (Diff revision 2) > - expect(wrapper.find("div.message.cm-s-mozilla span.message-body.devtools-monospace").length).toBe(1); > + expect(wrapper > + .find("div.message.cm-s-mozilla span.message-body.devtools-monospace").length This formatting is really odd. Maybe: ``` let selector = "..."; expect(wrapper.find(selector).length).toBe(1); ``` ::: devtools/client/webconsole/new-console-output/test/components/network-event-message.test.js:78 (Diff revision 2) > - expect(wrapper.find("div.message.cm-s-mozilla span.message-body.devtools-monospace").length).toBe(1); > + expect(wrapper > + .find("div.message.cm-s-mozilla span.message-body.devtools-monospace").length > + ).toBe(1); Same here. ::: devtools/client/webconsole/new-console-output/test/fixtures/serviceContainer.js:16 (Diff revision 2) > + // eslint-disable-next-line react/display-name > createElement: tagName => document.createElement(tagName) I don't see why this should be needed. Is it possible to remove the comment ? ::: devtools/client/webconsole/new-console-output/test/require-helper.js:42 (Diff revision 2) > case "Services.default": > return `module.exports = require("devtools/client/webconsole/new-console-output/test/fixtures/Services")`; > case "devtools/shared/client/main": > return `module.exports = require("devtools/client/webconsole/new-console-output/test/fixtures/ObjectClient")`; > } > + return undefined; Maybe something similar to `return path;` could work? If not, I'm fine with undefined.
Attachment #8817463 -
Flags: review?(ntim.bugs) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
Pushed by chevobbe.nicolas@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d4f99821f915 Fix ESLint in new console frontend; r=ntim
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d4f99821f915
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•