1488263 - Enable eslint in mailnews/extensions/newsblog

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, task)

Description

[alta88]

May as well get something started in /mailnews.

Comment 1

[alta88]

Attachment: newsblogLint.patch

Part 1, formatting changes.

Comment 2

[alta88]

Attachment: eslintrc-mailnews.patch

Part 2, enable eslint and add files. Very basic, just a start.

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[Jorg K (CEST = GMT+2)]

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.

Comment 5

[alta88]

(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.

Comment 6

[alta88]

Attachment: newsblogLint.patch

Comment 7

[alta88]

(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

[Jorg K (CEST = GMT+2)]

(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

[Jorg K (CEST = GMT+2)]

(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/

Comment 10

[alta88]

(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.

Comment 11

[alta88]

Attachment: newsblogLint.patch

Comment 12

[alta88]

Attachment: eslintrc-mailnews.patch

fixed.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 9006097 [details] [diff] [review]
eslintrc-mailnews.patch

Indeed.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Jorg K (CEST = GMT+2)]

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)

Comment 16

[alta88]

Attachment: lintfix.patch

fixes..

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: lintfix JK.patch

I came up with this. Which one is better?

Comment 18

[alta88]

Comment on attachment 9006113 [details] [diff] [review]
lintfix JK.patch

let's go with your file exclusion.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/40d5e3c09200
Follow-up: fix linting problems. r=alta88 DONTBUILD

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9006111 [details] [diff] [review]
lintfix.patch

OK, thanks.

Comment 21

[alta88]

Attachment: patch

silly typo; lint fix caused logic change.

sheriff, i hope it's ok to fix it this way; if not let me know.

Comment 22

[Jorg K (CEST = GMT+2)]

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.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dac8ee26189f
Follow-up: Fix logic error introduced in rev 0b3d20e86288 during linting. r=jorgk DONTBUILD

Comment 24

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/3c660ddf4c60dbab83043c5ea46bf54c94e5e65f