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)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 65.0
People
(Reporter: darktrojan, Assigned: darktrojan)
Details
Attachments
(2 files, 1 obsolete file)
170.32 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #9027447 -
Flags: review?(acelists)
Assignee | ||
Comment 2•7 years 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 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 7•7 years 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 years 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 years 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 years 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 years 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 years 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 years 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 years 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 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•7 years ago
|
Target Milestone: --- → Thunderbird 65.0
Updated•6 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•