Enable eslint of browser/components/feeds/

RESOLVED FIXED in Firefox 52

Status

()

Firefox
RSS Discovery and Preview
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: standard8, Assigned: jordan, Mentored)

Tracking

unspecified
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [lang=js])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
To help with preventing errors in coding, we should enable eslint in browser/components/feeds/

To enable checking of files, remove the `browser/components/feeds/**` entry from the top-level `.eslintignore`.

Then you can run eslint on the directory with

  ./mach eslint browser/components/feeds

(note: eslint needs a one-time setup of `./mach eslint --setup`).

The errors listed in the output are the ones to be fixed.

Information about the eslint rules can be found here:

http://eslint.org/docs/rules/

If you are uncertain of how to fix a rule, please feel free to ask here or on irc (https://wiki.mozilla.org/Irc) in #fx-team (I'm on as Standard8).
(Assignee)

Comment 1

8 months ago
hi Mark, please assign this bug to me. Thanks!
Comment hidden (mozreview-request)
(Reporter)

Comment 3

8 months ago
mozreview-review
Comment on attachment 8805008 [details]
Bug 1311343 - Enable eslint of browser/components/feeds/;

https://reviewboard.mozilla.org/r/88834/#review88020

Thank you for taking this on. Great work so far. There's one minor change to do.

::: browser/components/feeds/test/test_bug436801.html:101
(Diff revision 1)
>      var nsTable = {
>        xml: "http://www.w3.org/XML/1998/namespace",
>      };
>      for (var name in attrSchema) {
>        var [ns, nsName] = name.split(":");
> -      var val = nsName ? node.getAttributeNS(nsTable[ns], nsName) :
> +      val = nsName ? node.getAttributeNS(nsTable[ns], nsName) :

We're generally changing to use let rather than var, as it defines scopes better.

Since both the `val` variables are within loops, could you change them both to be `let val = ...`. That'll also be clearer to future readers.
(Reporter)

Updated

8 months ago
Assignee: nobody → souravgarg833
Comment hidden (mozreview-request)
(Assignee)

Comment 5

8 months ago
please have a look at the updated version.
Also, should I just resolve rest of similar bugs like bug 1311345 & 1311347, or should I leave them for new contributors?
(Reporter)

Comment 6

8 months ago
mozreview-review
Comment on attachment 8805008 [details]
Bug 1311343 - Enable eslint of browser/components/feeds/;

https://reviewboard.mozilla.org/r/88834/#review88120

Great, thank you for the update Sourav.
Attachment #8805008 - Flags: review?(standard8) → review+
(Reporter)

Comment 7

8 months ago
(In reply to Sourav Garg [:jordan] from comment #5)
> please have a look at the updated version.

Done! (and on its way to landing).

> Also, should I just resolve rest of similar bugs like bug 1311345 & 1311347,
> or should I leave them for new contributors?

I've not heard of any offers for them yet, so you're welcome to pick those up. You're also welcome to have a look around for other bugs if you want.

Comment 8

8 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd858edfb90a
Enable eslint of browser/components/feeds/; r=standard8

Comment 9

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd858edfb90a
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.