Closed Bug 1317069 Opened 8 years ago Closed 8 years ago

Fix eslint for new console output

Categories

(DevTools :: Console, defect, P3)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: ntim, Assigned: nchevobbe)

References

Details

Attachments

(1 file)

Bug 1317069 - Fix ESLint in new console frontend;
58 bytes, text/x-review-board-request
ntim
: review+
Details 
      No description provided.
Blocks: devtools-eslint
Priority: -- → P3
Assignee: nobody → chevobbe.nicolas
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 !
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
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.
(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 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+
Pushed by chevobbe.nicolas@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d4f99821f915
Fix ESLint in new console frontend; r=ntim
https://hg.mozilla.org/mozilla-central/rev/d4f99821f915
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: