Bug 1509779

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

RESOLVED FIXED in Thunderbird 65.0

Get help with this page

Status

Product:

Component:

Type:

enhancement

Status:

RESOLVED FIXED

Opened:

7 months ago

Updated:

7 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

Version:

unspecified

Target:

Thunderbird 65.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

1509779-eslint-old-rules-1.diff 7 months ago Geoff Lankow (:darktrojan) 39.50 KB, patch	aceman : review+	Details \| Diff \| Splinter Review
1509779-eslint-new-rules-1.diff 7 months ago Geoff Lankow (:darktrojan) 170.32 KB, patch	aceman : review+	Details \| Diff \| Splinter Review
1509779-eslint-old-rules-2.diff 7 months ago Geoff Lankow (:darktrojan) 15.44 KB, patch	aceman : review+	Details \| Diff \| Splinter Review

Geoff Lankow (:darktrojan)

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.

Geoff Lankow (:darktrojan)

Assignee

Comment 1

•

7 months ago

Posted patch 1509779-eslint-old-rules-1.diff (obsolete) — Details — Splinter Review

Attachment #9027447 - Flags: review?(acelists)

Geoff Lankow (:darktrojan)

Assignee

Comment 2

•

7 months ago

Posted patch 1509779-eslint-new-rules-1.diff — Details — Splinter Review

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)

:aceman

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)

:aceman

Comment 4

•

7 months ago

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

:aceman

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?

:aceman

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+

Jorg K (GMT+2)

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)

Geoff Lankow (:darktrojan)

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…

Geoff Lankow (:darktrojan)

Assignee

Comment 9

•

7 months ago

Posted patch 1509779-eslint-old-rules-2.diff — Details — Splinter Review

Largely the same as discussed before, but forgetting about the comment-spacing thing.

Attachment #9027447 - Attachment is obsolete: true

Attachment #9027824 - Flags: review?(acelists)

Geoff Lankow (:darktrojan)

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.

:aceman

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?

Geoff Lankow (:darktrojan)

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.

:aceman

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+

Pulsebot

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

Geoff Lankow (:darktrojan)

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.

Bugzilla

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

Status

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Security

(public)

User Story

Attachments

(2 attachments, 1 obsolete attachment)

Description

Comment 1

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Comment 14

Updated