Closed
Bug 1358050
Opened 7 years ago
Closed 7 years ago
Add no-implied-eval rule to eslint config
Categories
(Developer Infrastructure :: Lint and Formatting, enhancement)
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: freddy, Assigned: freddy)
References
()
Details
Attachments
(1 file)
We discourage use of eval (and string arguments to setInterval/setTimeout). I suggest we set this in stone by adding this to our eslint config.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → fbraun
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Looks good on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b2bf36674c7 (though MozReview thinks otherwise *shrugs*)
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 4•7 years ago
|
||
Tom: This eslint-rule is currently force-disabled in devtools/, would you be OK with enabling it as part of this bug? FWIW, enabling the rule locally causes no errors or warnings.
Flags: needinfo?(ttromey)
Comment 5•7 years ago
|
||
(In reply to Frederik Braun [:freddyb] from comment #4) > Tom: This eslint-rule is currently force-disabled in devtools/, would you be > OK with enabling it as part of this bug? > FWIW, enabling the rule locally causes no errors or warnings. Yes, I think it's a good idea. From the comment in devtools/.eslintrc.js, I'd imagine that this rule was disabled by accident, especially considering that "eval" itself is an error.
Flags: needinfo?(ttromey)
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8859923 [details] Bug 1358050: Add no-implied-eval rule to eslint config https://reviewboard.mozilla.org/r/131980/#review134934 Thanks for the patch. This looks good to me overall. nit: Unless there's a specific coding style for files in the toolkit/components/places/tests/browser folder, I would use 2 space indent, but I don't think this really matters for these test files. Please enable the rule for devtools, and I would suggest requesting a final review from :Standard8. ::: browser/components/preferences/languages.js:22 (Diff revision 2) > > // Ugly hack used to trigger extra reflow in order to work around XUL bug 1194844; > // see bug 1194346. > forceReflow() { > this._activeLanguages.style.fontKerning = "none"; > - setTimeout("gLanguagesDialog._activeLanguages.style.removeProperty('font-kerning')", 0); > + setTimeout(_ => { nit: when there's no parameter we usually do '() =>' rather than defining a dummy parameter.
Attachment #8859923 -
Flags: review?(florian)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8859923 -
Flags: review?(florian)
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8859923 [details] Bug 1358050: Add no-implied-eval rule to eslint config https://reviewboard.mozilla.org/r/131980/#review135214 Looks good, thanks for doing this. r=Standard8
Attachment #8859923 -
Flags: review?(standard8) → review+
Comment 10•7 years ago
|
||
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/402503d06db8 Add no-implied-eval rule to eslint config r=standard8
Keywords: checkin-needed
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/402503d06db8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Product: Testing → Firefox Build System
Updated•5 years ago
|
Version: Version 3 → 3 Branch
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•