Closed Bug 1509779 Opened 7 years ago Closed 7 years ago

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

Categories

(Thunderbird :: General, task)

Product:
Thunderbird
Component:
General
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 65.0

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

Attachments

(2 files, 1 obsolete file)

1509779-eslint-new-rules-1.diff
170.32 KB, patch
aceman
: review+
Details | Diff | Splinter Review
1509779-eslint-old-rules-2.diff
15.44 KB, patch
aceman
: review+
Details | Diff | Splinter Review
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.
Attached patch 1509779-eslint-old-rules-1.diff (obsolete) — — Splinter Review
Attachment #9027447 - Flags: review?(acelists)
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 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)
Could you also add the rule to notify us when something can be replaced by Services.* ?
(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 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 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)
(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…
Largely the same as discussed before, but forgetting about the comment-spacing thing.
Attachment #9027447 - Attachment is obsolete: true
Attachment #9027824 - Flags: review?(acelists)
(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.
(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?
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 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+
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 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: