Closed Bug 1542666 Opened 2 years ago Closed 2 years ago

Turn on ESLint in editor

Categories

(MailNews Core :: Composition, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 68.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(2 files)

I've done the linting in editor, ignoring most of the parts Thunderbird doesn't ship.

These changes were done by eslint --fix. Not asking for review, as nobody who values their sanity wants to review a computer-generated 22000-line patch.

Good luck, and remember, most of this code never sees the light of day, so don't be too picky.

Attachment #9056449 - Flags: review?(acelists)
Comment on attachment 9056449 [details] [diff] [review]
1542666-eslint-editor-manual-1.diff

Review of attachment 9056449 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: editor/ui/composer/content/ComposerCommands.js
@@ +1443,5 @@
>    }
>    return false;
>  }
>  
> +/* eslint-disable */

Disable whole linting? Why?

@@ +1729,5 @@
>      goDoCommand(command);
>    }
>  }
>  
> +/* eslint-disable */

Again?

::: editor/ui/composer/content/editor.js
@@ -556,5 @@
>        return false;
>      }
>  
>      // Save to local disk
> -    return await SaveDocument(false, false, editor.contentsMIMEType);

Was this 'await' wrong? Does eslint find this stuff?

@@ +2788,5 @@
>    var editor = GetCurrentEditor();
>    if (!editor) return null;
>  
>    try {
>      var selection = editor.selection;

'selection' is declared int he block, but used outside.

::: editor/ui/composer/content/editorUtilities.js
@@ -279,5 @@
>  /** *********** General editing command utilities ***************/
>  
>  function GetDocumentTitle() {
>    try {
> -    return new XPCNativeWrapper(GetCurrentEditor().document, "title").title;

Found by eslint?

::: editor/ui/dialogs/content/EdConvertToTable.js
@@ +286,5 @@
>              editor.selection.collapse(node2, 0);
>            } catch (e) {}
>            break;
>          }
> +        node2 = node2.nextSibling;

Heh, nice.

::: editor/ui/dialogs/content/EdDialogTemplate.js
@@ +22,5 @@
>    // gDialog is declared in EdDialogCommon.js
>    // Set commonly-used widgets like this:
>    gDialog.fooButton = document.getElementById("fooButton");
>  
> +  InitDialog();

Nice ;)

::: editor/ui/dialogs/content/EdReplace.js
@@ +70,5 @@
>    // get the find service, which stores global find state
>      gFindService = Cc["@mozilla.org/find/find_service;1"]
>                           .getService(Ci.nsIFindService);
> +  } catch (e) {
> +    dump("No find service!\n"); gFindService = 0;

2 commands

@@ +334,5 @@
>          selecRange = selection.getRangeAt(0);
>        }
>      }
> +  } catch (e) {
> +  }

At other places you left this as 'catch (e) {}'.

::: editor/ui/dialogs/content/EdTableProps.js
@@ -657,5 @@
>          gActiveEditor.selectTableColumn();
>          break;
>      }
> -    // Get number of cells selected
> -    var tableOrCellElement = gActiveEditor.getSelectedOrParentTableElement(tagNameObj, countObj);

The countObj is used below. Maybe you only wanted to kill tableOrCellElement ?
Attachment #9056449 - Flags: review?(acelists) → review+

(In reply to :aceman from comment #4)

Disable whole linting? Why?
Again?

I said why, in the very first chunk of the diff. We don't use this code (I wish it would be chopped out and put in another file, but that's not going to happen anytime soon) and it's not worth my time hunting down the errors in it.

::: editor/ui/composer/content/editor.js
@@ -556,5 @@

  • return await SaveDocument(false, false, editor.contentsMIMEType);

Was this 'await' wrong? Does eslint find this stuff?

Not wrong, just pointless. The async function returns a Promise, and SaveDocument returns a Promise. There's probably no difference at all at runtime.

::: editor/ui/composer/content/editorUtilities.js
@@ -279,5 @@

/** *********** General editing command utilities ***************/

function GetDocumentTitle() {
try {

  • return new XPCNativeWrapper(GetCurrentEditor().document, "title").title;

Found by eslint?

Yeah, I don't think XPCNativeWrapper is really a thing any more. Anyway, I just accessed the document the same way SetDocumentTitle does, so I think this okay.

::: editor/ui/dialogs/content/EdTableProps.js
@@ -657,5 @@

     gActiveEditor.selectTableColumn();
     break;
 }
  • // Get number of cells selected
  • var tableOrCellElement = gActiveEditor.getSelectedOrParentTableElement(tagNameObj, countObj);

The countObj is used below. Maybe you only wanted to kill tableOrCellElement
?

Well spotted, I didn't realise that was an out-arg.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/2f1b233c4f65
Turn on ESLint in editor (automatic changes by eslint --fix); rs=me
https://hg.mozilla.org/comm-central/rev/09d757485ede
Turn on ESLint in editor (manual changes); r=aceman

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
Blocks: 1549166
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.