Closed
Bug 1160145
Opened 9 years ago
Closed 9 years ago
Fix eslint warnings in Loop's xpcshell and mochitest files and turn on linting for them
Categories
(Hello (Loop) :: Client, defect, P3)
Hello (Loop)
Client
Tracking
(firefox40 fixed)
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(2 files, 1 obsolete file)
20.85 KB,
patch
|
Details | Diff | Splinter Review | |
48.39 KB,
patch
|
Details | Diff | Splinter Review |
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+
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
... 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)
Assignee | ||
Updated•9 years ago
|
Attachment #8599874 -
Flags: review?(dmose)
Comment 4•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8599871 -
Flags: review+
Comment 5•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8599874 -
Flags: review?(dmose)
Comment 6•9 years ago
|
||
Oh, I think I see what happened: it effectively goes with either one. r=dmose :-)
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f27feb597c05
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•