Closed Bug 1160145 Opened 5 years ago Closed 5 years ago

Fix eslint warnings in Loop's xpcshell and mochitest files and turn on linting for them

Categories

(Hello (Loop) :: Client, defect, P3)

defect
Points:
2

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- fixed
Blocking Flags:
backlog tech-debt

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(2 files, 1 obsolete file)

The xpcshell and mochitest files are currently ignored for eslint. I'd like to get thme turned on soon, so that as we enable more rules we ensure those files are covered.
Flags: qe-verify-
Flags: firefox-backlog+
This gets the files working. Most of this is "use strict" additions or minor fixes.

browser_mozLoop_pluralStrings.js is a full change because it was using windows line endings.

test_looppush_initialize.js had everything unnecessarily wrapped in braces (which eslint didn't like), so I've removed those and re-indented.

I'll attach a diff -w in a mo.

I don't mind who reviews this, I'll take whoever gets there first ;-)
Attachment #8599865 - Flags: review?(mdeboer)
Attachment #8599865 - Flags: review?(dmose)
Attached patch diff -wSplinter Review
... and now with the 2 new .eslintrc files that I'd forgotten to git add.
Attachment #8599865 - Attachment is obsolete: true
Attachment #8599865 - Flags: review?(mdeboer)
Attachment #8599865 - Flags: review?(dmose)
Attachment #8599874 - Flags: review?(dmose)
Comment on attachment 8599871 [details] [diff] [review]
diff -w

Review of attachment 8599871 [details] [diff] [review]:
-----------------------------------------------------------------

If you could fix loop/README.txt with s/-ext/--ext/g, that'd be great.  It tripped me up when I was testing this.

r=dmose, with any appropriate tweaks.

::: browser/components/loop/test/mochitest/head.js
@@ +126,4 @@
>    loopPanel.setAttribute("animate", "false");
>  
>    // Now get the actual API.
> +  return promiseGetMozLoopAPI();

Given the comment above this function, would leaving the yield alone and changing the function to function* be a better fix?

(I don't actually know if there's a real problem here or not).
Attachment #8599871 - Flags: review+
So I'm confused as to whether the "diff -w" goes with v1 or v2 of the patch.  If it goes with v2, please consider v2 r+ed, with the comments above.  If it goes with v1, can you generate a new diff -w?
Attachment #8599874 - Flags: review?(dmose)
Oh, I think I see what happened: it effectively goes with either one.  r=dmose  :-)
(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #4)
> >    // Now get the actual API.
> > +  return promiseGetMozLoopAPI();
> 
> Given the comment above this function, would leaving the yield alone and
> changing the function to function* be a better fix?

I checked, and removing the yield is the right thing to do - promiseGetMozLoopAPI loads into a global variable, rather than resolving with it, so I've updated the comment.
https://hg.mozilla.org/mozilla-central/rev/f27feb597c05
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.