Closed Bug 1248563 Opened 4 years ago Closed 4 years ago

eslint cleanup of storage inspector code

Categories

(DevTools :: Storage Inspector, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(1 file)

Just removed a few lines of dead code and made existing code conform to our eslint setup.
The failed jobs are unrelated, known intermittents.
Comment on attachment 8719821 [details]
MozReview Request: Bug 1248563 - eslint cleanup of storage inspector code r=pbrosset

https://reviewboard.mozilla.org/r/35133/#review31917

Mostly good to go. My most important comment is about not disabling eslint entirely in head.js but instead coniguring the unused rule (which is what we're doing for other head.js files).
No need to re-request review after the changed have been done.
One question: with these changes, can the storage inspector be un-ignored from .eslintignore, or are there other things left to clean up?
If so, can you file another bug (good-first, mentored) to finish the cleanup?

::: devtools/client/shared/widgets/TableWidget.js:858
(Diff revision 1)
> -      return;
> +      return true;
>      }
>  
>      if (event.button == 0 && event.originalTarget == this.header) {
>        return this.table.sortBy(this.id);

Why does this event handler callback even return a value at all?
`sortBy` doesn't seem to return anything useful anyway.
ESLint complains about multiple returns when they don't seem to be of the same type.
But this would pass ESLint rules and, I think, be correct:

```
if (event.originalTarget == this.column) {
  return;
}

if (event.button == 0 && event.originalTarget == this.header) {
  this.table.sortBy(this.id)
}
```

::: devtools/client/shared/widgets/TreeWidget.js:487
(Diff revision 1)
> +                items[this.level];

nit: maybe put each condition on its own line?
```
let label = items[this.level].label ||
            items[this.level].id ||
            items[this.level];
```

::: devtools/client/storage/panel.js:1
(Diff revision 1)
> -/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ // eslint-disable-line

I'm not a fan of adding yet another comment at the end of this already long line.
Can't this emacs config line be wrapped on 2 lines?

If not, I would advise changing our .eslintrc instead by adding another regex part in our max-len ignorePattern regex:

`"max-len": [1, 80, 2, {"ignoreUrls": true, "ignorePattern": "\\s*require\\s*\\(|^\\s*loader\\.lazy|-\\*- Mode"}],`

::: devtools/client/storage/test/head.js:7
(Diff revision 1)
> +/* eslint-disable */

I'm guessing you've disabled this whole block because eslint complains about unused globals, right?
If so, there's a better solution than just disabling eslint altogether. Add this comment to the top of this head.js file:

```
/* eslint no-unused-vars: [2, {"vars": "local"}] */
```

This makes sure eslint doesn't complain about unused globals variables in this file (but will still complain about unused local variables).

::: devtools/client/storage/test/head.js:8
(Diff revision 1)
>  var { console } = Cu.import("resource://gre/modules/Console.jsm", {});

I think you don't need to import console in tests, it's already defined by the test runner.

::: devtools/client/storage/test/head.js:90
(Diff revision 1)
> -function* openTabAndSetupStorage(url) {
> +function* openTabAndSetupStorage(url) { // eslint-disable-line

This can then be removed.

::: devtools/client/storage/test/head.js:200
(Diff revision 1)
> -function* finishTests() {
> +function* finishTests() { // eslint-disable-line

And this.

::: devtools/client/storage/test/head.js:295
(Diff revision 1)
> -  function fetchError(aProp) {
> +  function fetchError(prop) { // eslint-disable-line

And this.

::: devtools/client/storage/test/head.js:328
(Diff revision 1)
> -function findVariableViewProperties(aRules, aParsed) {
> +function findVariableViewProperties(ruleArray, parsed) { // eslint-disable-line

And this.

::: devtools/client/storage/test/head.js:488
(Diff revision 1)
> -function* selectTreeItem(ids) {
> +function* selectTreeItem(ids) { // eslint-disable-line

And this.

::: devtools/client/storage/test/head.js:509
(Diff revision 1)
> -function* selectTableItem(id) {
> +function* selectTableItem(id) { // eslint-disable-line

And this.

::: devtools/client/storage/test/head.js:526
(Diff revision 1)
> -function once(target, eventName, useCapture=false) {
> +function once(target, eventName, useCapture = false) { // eslint-disable-line

And this!

::: devtools/server/actors/storage.js:1808
(Diff revision 1)
> -      this.childActorPool.set(store, new actor(this));
> +      // Let's use an upper-case constructor to avoid eslint errors.
> +      let Actor = actor;
> +      this.childActorPool.set(store, new Actor(this));

I find this a little easier to read (and no need for the eslint comment):

```
for (let [store, ActorConstructor] of storageTypePool) {
  this.childActorPool.set(store, new ActorConstructor(this));
}
```
Attachment #8719821 - Flags: review?(pbrosset) → review+
Comment on attachment 8719821 [details]
MozReview Request: Bug 1248563 - eslint cleanup of storage inspector code r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35133/diff/1-2/
Attachment #8719821 - Attachment description: MozReview Request: Bug 1248563 - eslint cleanup of storage inspector code r?=pbrosset → MozReview Request: Bug 1248563 - eslint cleanup of storage inspector code r=pbrosset
https://hg.mozilla.org/mozilla-central/rev/8b188df8101e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Blocks: 1251720
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.