Closed
Bug 1488263
Opened 6 years ago
Closed 6 years ago
Enable eslint in mailnews/extensions/newsblog
Categories
(MailNews Core :: Feed Reader, task)
MailNews Core
Feed Reader
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 63.0
People
(Reporter: alta88, Assigned: alta88)
Details
Attachments
(4 files, 4 obsolete files)
331.54 KB,
patch
|
alta88
:
review+
|
Details | Diff | Splinter Review |
2.64 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
alta88
:
feedback+
|
Details | Diff | Splinter Review |
1.09 KB,
patch
|
alta88
:
review+
|
Details | Diff | Splinter Review |
May as well get something started in /mailnews.
Part 1, formatting changes.
Assignee: nobody → alta88
Attachment #9006069 -
Flags: review?(jorgk)
Part 2, enable eslint and add files. Very basic, just a start.
Attachment #9006070 -
Flags: review?(jorgk)
Comment 3•6 years ago
|
||
Comment on attachment 9006069 [details] [diff] [review] newsblogLint.patch Review of attachment 9006069 [details] [diff] [review]: ----------------------------------------------------------------- My attention level dropped during the second half, but I still found some stuff. ::: mailnews/extensions/newsblog/content/feed-subscriptions.js @@ +16,5 @@ > > get mTree() { return document.getElementById("rssSubscriptionsList"); }, > > mFeedContainers: [], > + mRSSServer: null, What about the multiple spaces here? They are allowed in objects? @@ +587,5 @@ > null; > let open = !aFolder.isServer && > aFolder.server == this.mRSSServer && > + this.mActionMode == this.kImportingOPML; > + let folderObject = { children: [], Multiple spaces? @@ +1492,5 @@ > options.category.prefixEnabled = document.getElementById("autotagUsePrefix").checked; > options.category.prefix = document.getElementById("autotagPrefix").value; > } > > + let feedProperties = { feedName: name, Not much point to have the spaces here. But your call. @@ +1494,5 @@ > } > > + let feedProperties = { feedName: name, > + feedLocation, > + feedFolder: addFolder, And here. @@ +1760,2 @@ > let parentIndex, parentItem, newItem, level; > +// let rows = win.mFeedContainers; What happened here? @@ +2035,2 @@ > feedWindow.selectFeed({ folder: curSelItem.parentFolder, > url: curSelItem.url }, parentIndex); Still ugly here. @@ +2544,1 @@ > let folderOutlines = 0; What is that about? ::: mailnews/extensions/newsblog/content/newsblogOverlay.js @@ +200,1 @@ > * 'messagepane', 'browser', 'tab', 'window'. Should have shifted this one, too. @@ +330,3 @@ > subject = "Re: " + subject; > + } else if (type == msgComposeType.ForwardInline || > + type == msgComposeType.ForwardAsAttachment) { Misaligned.
Attachment #9006069 -
Flags: review?(jorgk) → review+
Comment 4•6 years ago
|
||
Comment on attachment 9006070 [details] [diff] [review] eslintrc-mailnews.patch Review of attachment 9006070 [details] [diff] [review]: ----------------------------------------------------------------- ::: .eslintignore @@ +30,2 @@ > mailnews/** > +!mailnews/extensions/newsblog Something wrong here. I can do: ../mach eslint mail/components/about-support but I can't do ../mach eslint mailnews/extensions/newsblog/. I think you need to list directories separately as mail/ does.
Attachment #9006070 -
Flags: review?(jorgk)
(In reply to Jorg K (GMT+2) from comment #3) > Comment on attachment 9006069 [details] [diff] [review] > newsblogLint.patch > > Review of attachment 9006069 [details] [diff] [review]: > ----------------------------------------------------------------- > > My attention level dropped during the second half, but I still found some > stuff. > > ::: mailnews/extensions/newsblog/content/feed-subscriptions.js > @@ +16,5 @@ > > > > get mTree() { return document.getElementById("rssSubscriptionsList"); }, > > > > mFeedContainers: [], > > + mRSSServer: null, > > What about the multiple spaces here? They are allowed in objects? > > @@ +587,5 @@ > > null; > > let open = !aFolder.isServer && > > aFolder.server == this.mRSSServer && > > + this.mActionMode == this.kImportingOPML; > > + let folderObject = { children: [], > > Multiple spaces? rule key-spacing disallows space before the : but spaces after work and this is really needed for readability. > > @@ +1492,5 @@ > > options.category.prefixEnabled = document.getElementById("autotagUsePrefix").checked; > > options.category.prefix = document.getElementById("autotagPrefix").value; > > } > > > > + let feedProperties = { feedName: name, > > Not much point to have the spaces here. But your call. > > @@ +1494,5 @@ > > } > > > > + let feedProperties = { feedName: name, > > + feedLocation, > > + feedFolder: addFolder, > > And here. > yeah, not much, fixed. > @@ +1760,2 @@ > > let parentIndex, parentItem, newItem, level; > > +// let rows = win.mFeedContainers; > > What happened here? > removed. > @@ +2035,2 @@ > > feedWindow.selectFeed({ folder: curSelItem.parentFolder, > > url: curSelItem.url }, parentIndex); > > Still ugly here. tweaked. > > @@ +2544,1 @@ > > let folderOutlines = 0; > > What is that about? the parser can't find the var within the function for some reason, maybe because it's nested within 2 try loops. > > ::: mailnews/extensions/newsblog/content/newsblogOverlay.js > @@ +200,1 @@ > > * 'messagepane', 'browser', 'tab', 'window'. > > Should have shifted this one, too. > > @@ +330,3 @@ > > subject = "Re: " + subject; > > + } else if (type == msgComposeType.ForwardInline || > > + type == msgComposeType.ForwardAsAttachment) { > > Misaligned. both fixed.
Attachment #9006069 -
Attachment is obsolete: true
Attachment #9006091 -
Flags: review+
(In reply to Jorg K (GMT+2) from comment #4) > Comment on attachment 9006070 [details] [diff] [review] > eslintrc-mailnews.patch > > Review of attachment 9006070 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: .eslintignore > @@ +30,2 @@ > > mailnews/** > > +!mailnews/extensions/newsblog > > Something wrong here. I can do: > ../mach eslint mail/components/about-support > but I can't do > ../mach eslint mailnews/extensions/newsblog/. > > I think you need to list directories separately as mail/ does. that would be sad. did you build? it wfm, from obj-dir, using ../../mach lint ../mailnews/extensions/newsblog
Comment 8•6 years ago
|
||
(In reply to alta88 from comment #5) > > > let folderOutlines = 0; > > What is that about? > the parser can't find the var within the function for some reason, maybe > because it's nested within 2 try loops. No, it's unused, ie. write only. It can go.
Comment 9•6 years ago
|
||
(In reply to alta88 from comment #7) > that would be sad. did you build? it wfm, from obj-dir, using > ../../mach lint ../mailnews/extensions/newsblog Sad, but true. We want to lint from the source directory, see bug 1487832 comment #9: ../mach eslint mailnews/extensions/newsblog/
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #8) > (In reply to alta88 from comment #5) > > > > let folderOutlines = 0; > > > What is that about? > > the parser can't find the var within the function for some reason, maybe > > because it's nested within 2 try loops. > No, it's unused, ie. write only. It can go. you're right.
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #9006091 -
Attachment is obsolete: true
Attachment #9006095 -
Flags: review+
Assignee | ||
Comment 12•6 years ago
|
||
fixed.
Attachment #9006070 -
Attachment is obsolete: true
Attachment #9006097 -
Flags: review?(jorgk)
Comment 13•6 years ago
|
||
Comment on attachment 9006097 [details] [diff] [review] eslintrc-mailnews.patch Indeed.
Attachment #9006097 -
Flags: review?(jorgk) → review+
Keywords: checkin-needed
Comment 14•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/0b3d20e86288 Enable eslint in mailnews/extensions/newsblog, Part 1: format changes. r=jorgk https://hg.mozilla.org/comm-central/rev/35b461974568 Enable eslint in mailnews/extensions/newsblog, Part 2: eslint changes. r=jorgk
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 63.0
Comment 15•6 years ago
|
||
Care to fix the bustage? TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mailnews/.eslintrc.js:3:1 | 'module' is not defined. (no-undef) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mailnews/mailnews.js:166:1 | Parsing error: Unexpected character '#' (eslint)
Flags: needinfo?(alta88)
Assignee | ||
Comment 16•6 years ago
|
||
fixes..
Flags: needinfo?(alta88)
Attachment #9006111 -
Flags: review?(jorgk)
Comment 17•6 years ago
|
||
I came up with this. Which one is better?
Attachment #9006113 -
Flags: feedback?(alta88)
Assignee | ||
Comment 18•6 years ago
|
||
Comment on attachment 9006113 [details] [diff] [review] lintfix JK.patch let's go with your file exclusion.
Attachment #9006113 -
Flags: feedback?(alta88) → feedback+
Comment 19•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/40d5e3c09200 Follow-up: fix linting problems. r=alta88 DONTBUILD
Comment 20•6 years ago
|
||
Comment on attachment 9006111 [details] [diff] [review] lintfix.patch OK, thanks.
Attachment #9006111 -
Attachment is obsolete: true
Attachment #9006111 -
Flags: review?(jorgk)
Assignee | ||
Comment 21•5 years ago
|
||
silly typo; lint fix caused logic change.
sheriff, i hope it's ok to fix it this way; if not let me know.
Flags: needinfo?(jorgk)
Attachment #9060200 -
Flags: review+
Keywords: checkin-needed
Comment 22•5 years ago
|
||
Long live linting!
https://hg.mozilla.org/comm-central/rev/0b3d20e86288#l8.145
Not ideal to land it here, but we don't need to burn a new bug for a one liner.
Flags: needinfo?(jorgk)
Comment 23•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/dac8ee26189f
Follow-up: Fix logic error introduced in rev 0b3d20e86288 during linting. r=jorgk DONTBUILD
Keywords: checkin-needed
Comment 24•5 years ago
|
||
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•