Enable eslint in mailnews/extensions/newsblog

RESOLVED FIXED in Thunderbird 63.0

Status

enhancement
RESOLVED FIXED
11 months ago
3 months ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

unspecified
Thunderbird 63.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

May as well get something started in /mailnews.
Posted patch newsblogLint.patch (obsolete) — Splinter Review
Part 1, formatting changes.
Assignee: nobody → alta88
Attachment #9006069 - Flags: review?(jorgk)
Component: Database → Feed Reader
Posted patch eslintrc-mailnews.patch (obsolete) — Splinter Review
Part 2, enable eslint and add files. Very basic, just a start.
Attachment #9006070 - Flags: review?(jorgk)
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 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.
Posted patch newsblogLint.patch (obsolete) — Splinter Review
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
(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.
(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/
(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.
Attachment #9006091 - Attachment is obsolete: true
Attachment #9006095 - Flags: review+
fixed.
Attachment #9006070 - Attachment is obsolete: true
Attachment #9006097 - Flags: review?(jorgk)
Comment on attachment 9006097 [details] [diff] [review]
eslintrc-mailnews.patch

Indeed.
Attachment #9006097 - Flags: review?(jorgk) → review+
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Closed: 11 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
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)
Posted patch lintfix.patch (obsolete) — Splinter Review
fixes..
Flags: needinfo?(alta88)
Attachment #9006111 - Flags: review?(jorgk)
I came up with this. Which one is better?
Attachment #9006113 - Flags: feedback?(alta88)
Comment on attachment 9006113 [details] [diff] [review]
lintfix JK.patch

let's go with your file exclusion.
Attachment #9006113 - Flags: feedback?(alta88) → feedback+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/40d5e3c09200
Follow-up: fix linting problems. r=alta88 DONTBUILD
Comment on attachment 9006111 [details] [diff] [review]
lintfix.patch

OK, thanks.
Attachment #9006111 - Attachment is obsolete: true
Attachment #9006111 - Flags: review?(jorgk)
Posted patch patchSplinter Review

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

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)

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
You need to log in before you can comment on or make changes to this bug.