Remove exceptions to the recommended linting rules, codify some unwritten rules

RESOLVED FIXED in Thunderbird 65.0

Status

enhancement
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
Thunderbird 65.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

7 months ago
Our top-level .eslintrc.js file has some exceptions to the recommended rules which don't need to exist. I also want to add some rules which we obey but aren't in the recommended rules.
Assignee

Comment 1

7 months ago
Attachment #9027447 - Flags: review?(acelists)
Assignee

Comment 2

7 months ago
In addition to the three rules added, I used ESLint to find (I think) all the remaining if/else blocks that had curly brackets on one branch but not others. I'm not going to enforce that rule because reviewers should prevent any new instances, and also it's a rule that I've modified for the purpose.
Attachment #9027449 - Flags: review?(acelists)

Comment 3

7 months ago
Comment on attachment 9027447 [details] [diff] [review]
1509779-eslint-old-rules-1.diff

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

Thanks.

::: common/bindings/textbox.xml
@@ +129,5 @@
>        <method name="_setNewlineHandling">
>          <body><![CDATA[
>            var str = this.getAttribute("newlines");
>            if (str && this.editor) {
> +            const nsIPlaintextEditor = Ci.nsIPlaintextEditor;

We could kill this constant.

::: mail/base/content/mailWidgets.xml
@@ +1712,5 @@
>                searchValue.junkPercent = children[9].value;
>              } else if (searchAttribute == nsMsgSearchAttrib.Size) {
>                searchValue.size = children[9].value;
>              } else if (searchAttribute == nsMsgSearchAttrib.HasAttachmentStatus) {
> +              searchValue.status = 0x10000000; // 0x10000000 is MSG_FLAG_ATTACHMENT;

Not your fault, but there is no such named constant. Is it Ci.nsMsgMessageFlags.Attachment ? You could fix it while here if it isn't hard to find out. Is searchValue.status supposed to contain Ci.nsMsgMessageFlags values?

::: mail/base/content/nsDragAndDrop.js
@@ +321,5 @@
>        var transferData = { data: null };
>        try {
>          aDragDropObserver.onDragStart(aEvent, transferData, dragAction);
>        } catch (e) {
> +        return; // not a draggable item, bail!

This isn't a full sentence, but the trailing exclamation mark seems odd without also a leading capital letter. Jorg can judge this better.

::: mail/base/content/sanitize.js
@@ +35,5 @@
>      var errors = null;
>  
>      // Cache the range of times to clear
>      if (this.ignoreTimespan)
> +      var range = null; // If we ignore timespan, clear everything

Trailing dot please.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4700,4 @@
>    } else {
>      bucket.currentIndex = (bucket.itemCount > 0) ? // else: if attachments exist,
>                            (bucket.itemCount - 1) : // focus last item;
> +                          -1; // else: nothing to focus.

This is getting ugly in the new style :(

::: mail/components/im/content/imconversation.xml
@@ +932,3 @@
>              (event.keyCode != 13 && // Return
> +             event.keyCode != 8 && // Backspace
> +             event.keyCode != 46)) // Delete

These could use some of the KeyEvent.DOM_VK_* constants directly but there are more places with this so you do not need to do it here. I make a new bug.
Attachment #9027447 - Flags: review?(acelists)
Attachment #9027447 - Flags: review+
Attachment #9027447 - Flags: feedback?(jorgk)

Comment 4

7 months ago
Could you also add the rule to notify us when something can be replaced by Services.* ?

Comment 5

7 months ago
(In reply to Geoff Lankow (:darktrojan) from comment #2)
> In addition to the three rules added, I used ESLint to find (I think) all
> the remaining if/else blocks that had curly brackets on one branch but not
> others. I'm not going to enforce that rule because reviewers should prevent
> any new instances,

Why not? Reviewers may miss it so why not make the check automatic? Is it costly?

Comment 6

7 months ago
Comment on attachment 9027449 [details] [diff] [review]
1509779-eslint-new-rules-1.diff

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

Thanks.
Attachment #9027449 - Flags: review?(acelists) → review+

Comment 7

7 months ago
Comment on attachment 9027447 [details] [diff] [review]
1509779-eslint-old-rules-1.diff

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

Not totally happy with the comment/white-space changes. Unless I'm dreaming, I've seen a linter warning mandating two spaces before //. That's what I've been coding lately. Was that all wrong?

Also, making the comments line up shouldn't be disallowed.

::: mail/base/content/messageWindow.js
@@ +1237,5 @@
>          ClearPendingReadTimer();
>          gFolderDisplay.doCommand(Ci.nsMsgViewCommandType.markThreadRead);
>          return;
>        case "cmd_markAllRead":
> +        MsgMarkAllRead(); // Won't run since always disabled.

Doesn't the linter mandate two spaces before the //? That would be better.

::: mail/base/content/msgHdrView.js
@@ +248,5 @@
>    // Only offer openInTab and openInNewWindow if this window supports tabs.
>    let opensAreHidden = !document.getElementById("tabmail");
>    for (let id of ["otherActionsOpenInNewWindow", "otherActionsOpenInNewTab"]) {
>      let menu = document.getElementById(id);
> +    if (menu) { // May not be available yet.

And here.

::: mail/base/content/nsDragAndDrop.js
@@ +321,5 @@
>        var transferData = { data: null };
>        try {
>          aDragDropObserver.onDragStart(aEvent, transferData, dragAction);
>        } catch (e) {
> +        return; // not a draggable item, bail!

Leave it, we have lots of short comments like that. But what about the spaces before the //?
Attachment #9027447 - Flags: feedback?(jorgk)
Assignee

Comment 8

7 months ago
(In reply to Jorg K (GMT+1) from comment #7)
> Not totally happy with the comment/white-space changes. Unless I'm dreaming,
> I've seen a linter warning mandating two spaces before //. That's what I've
> been coding lately. Was that all wrong?

You're thinking of Python comments which must be  # like this.

I'm not convinced about that rule either. It's odd that we'd allow it where m-c wouldn't, but there are places where it looks better to line up comments. Hmm…
Assignee

Comment 9

7 months ago
Largely the same as discussed before, but forgetting about the comment-spacing thing.
Attachment #9027447 - Attachment is obsolete: true
Attachment #9027824 - Flags: review?(acelists)
Assignee

Comment 10

7 months ago
(In reply to :aceman from comment #4)
> Could you also add the rule to notify us when something can be replaced by
> Services.* ?

It's already active.

(In reply to :aceman from comment #5)
> (In reply to Geoff Lankow (:darktrojan) from comment #2)
> > In addition to the three rules added, I used ESLint to find (I think) all
> > the remaining if/else blocks that had curly brackets on one branch but not
> > others. I'm not going to enforce that rule because reviewers should prevent
> > any new instances,
> 
> Why not? Reviewers may miss it so why not make the check automatic? Is it
> costly?

It's a new rule so it involves either making our own ESLint plugin, or convincing somebody to add it to m-c (unlikely). I guess a third option would be to have it added to ESLint itself, which I think is highly unlikely. I just don't think it's worth my time.

Comment 11

7 months ago
(In reply to Geoff Lankow (:darktrojan) from comment #10)
> (In reply to :aceman from comment #4)
> > Could you also add the rule to notify us when something can be replaced by
> > Services.* ?
> 
> It's already active.

So why didn't it catch the cases I convert in bug 1508415? Or are those /mail folders except from eslint yet?
Assignee

Comment 12

7 months ago
In the case of nsDragAndDrop.js, it's because Ci.nsIScriptSecurityManager was assigned to a variable not passed directly to getService. In some other cases it's because XPCOMUtils.defineLazyServiceGetter[s] is used and it doesn't work on that, and the rest are not linted yet.

Comment 13

7 months ago
Comment on attachment 9027824 [details] [diff] [review]
1509779-eslint-old-rules-2.diff

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

Thanks.

::: mail/base/content/mailWidgets.xml
@@ +1818,5 @@
>          // initialize the junk score origin picker
>          this.initialize(document.getAnonymousNodes(this)[8], bundle);
>  
>          // initialize the tag list
> +        this.fillInTags();

Did this work even without 'this'?
Attachment #9027824 - Flags: review?(acelists) → review+

Comment 14

7 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/97d41ef7fee6
Stop ignoring mozilla/use-cc-etc linting rule, and other linting mistakes; r=aceman
https://hg.mozilla.org/comm-central/rev/f43611e42a79
Codify unwritten rules in ESLint configuration; r=aceman
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Assignee

Updated

7 months ago
Target Milestone: --- → Thunderbird 65.0
You need to log in before you can comment on or make changes to this bug.