Closed
Bug 1271084
Opened 8 years ago
Closed 8 years ago
Run ./mach eslint devtools/ --fix --no-ignore
Categories
(DevTools :: General, enhancement)
DevTools
General
Tracking
(firefox49 fixed)
RESOLVED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: ntim, Assigned: jryans)
References
Details
Attachments
(3 files, 3 obsolete files)
This would fix a lot of errors, like generator star spacing, space before () for anonymous functions, missing semi-colon, extra semi-colon, ... This would save a lot of time for people cleaning up the directory in the future. Patrick, thoughts ?
Reporter | ||
Updated•8 years ago
|
Blocks: devtools-eslint
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(pbrosset)
Reporter | ||
Comment 1•8 years ago
|
||
I use the --fix parameter systematically because it saves lots of time, and doesn't cause problems.
Reporter | ||
Comment 2•8 years ago
|
||
I had to split the patch in 3 parts, because the original was too big for bugzilla.
Reporter | ||
Comment 3•8 years ago
|
||
Reporter | ||
Comment 4•8 years ago
|
||
Comment 5•8 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #0) > This would fix a lot of errors, like generator star spacing, space before () > for anonymous functions, missing semi-colon, extra semi-colon, ... > > This would save a lot of time for people cleaning up the directory in the > future. > > Patrick, thoughts ? Sounds like a great idea, but I don't want to have to review this. Oops, you already flagged me for review :) Don't think I've seen multi-MB patches on bugzilla before. I think I'm going to have to avoid splinter altogether and find another to quickly review this. Can you please list the rules that --fix works on? If it's just spacing and semi-colons, I don't think I should review the code changes at all, and just R+ this.
Flags: needinfo?(pbrosset) → needinfo?(ntim.bugs)
Comment 6•8 years ago
|
||
(In reply to Patrick Brosset <:pbro> from comment #5) > Sounds like a great idea, but I don't want to have to review this. Oops, you > already flagged me for review :) No you didn't. Sorry I got confused by another review request I had received.
Assignee | ||
Comment 7•8 years ago
|
||
I have seen auto-fixing occasionally produce something suboptimal with spacing and line breaking, since the rules can't fully describing everything, and auto-fix only knows "do something to satisfy the rules I know about". But still, those cases are rare... we could still choose to just rubber stamp the changes, and manually fix up edge cases if we notice them later. Or someone can review the giant thing like usual. For me, trying to load even these smaller patches in Splinter makes the browser hang... If we want to review, we may need either MozReview (since it has pagination), or download the patch file for local inspection and leave any notes as just bug comments.
Assignee | ||
Updated•8 years ago
|
Severity: normal → enhancement
Comment 8•8 years ago
|
||
It might be nice to be more fine-grained about this. Run it specifically on each tool instead of the entire `client` directory. Would make it easier to at least scan.
Assignee | ||
Comment 9•8 years ago
|
||
I would be happy to review this, but it would be nice to have it in an easy to review form. I would prefer MozReview myself, which should be easier to scan since it has pagination for long commits...? But breaking it up per client directory could work as well.
Assignee | ||
Comment 10•8 years ago
|
||
I'll take this on, so we can have a consistent style in place. :tromey has offered to review.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 11•8 years ago
|
||
Let's see if MozReview can deal with one giant combined patch...
Assignee | ||
Updated•8 years ago
|
Attachment #8750037 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8750038 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8750039 -
Attachment is obsolete: true
Assignee | ||
Comment 12•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53282/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53282/
Attachment #8753457 -
Flags: review?(ttromey)
Attachment #8753458 -
Flags: review?(ttromey)
Assignee | ||
Comment 13•8 years ago
|
||
For simple rules like function spacing, we can auto-fix these across the code base so they are followed in a consistent way. To generate this patch, I ran: ./mach eslint devtools --no-ignore --fix After this, I reverted any changes to third party files that we really do want to ignore. Review commit: https://reviewboard.mozilla.org/r/53280/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53280/
Assignee | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7925b58b4b30
Comment 15•8 years ago
|
||
Comment on attachment 8753457 [details] MozReview Request: Bug 1271084 - Remove missing files from .eslintignore. r=tromey https://reviewboard.mozilla.org/r/53282/#review50086 Thank you.
Attachment #8753457 -
Flags: review?(ttromey) → review+
Comment 16•8 years ago
|
||
Comment on attachment 8753458 [details] MozReview Request: Bug 1271084 - Apply ESLint autofixes to ignored /devtools files. r=tromey https://reviewboard.mozilla.org/r/53280/#review50096 Thank you for this patch. I agree with the idea behind this patch, so r+. However there are a few issues I found while skimming the patch at high speed. I think at least these need to be addressed; ideally with some way to assure ourselves that we haven't missed similar issues. ::: devtools/client/debugger/test/mochitest/code_math.min.js:2 (Diff revision 1) > -function add(a,b,k){var result=a+b;return k(result)}function sub(a,b,k){var result=a-b;return k(result)}function mul(a,b,k){var result=a*b;return k(result)}function div(a,b,k){var result=a/b;return k(result)}function arithmetic(){add(4,4,function(a){sub(a,2,function(b){mul(b,3,function(c){div(c,2,function(d){console.log(d)})})})})}; > -//@ sourceMappingURL=code_math.map > +function add(a, b, k) {var result = a + b; return k(result);} function sub(a, b, k) {var result = a - b; return k(result);} function mul(a, b, k) {var result = a * b; return k(result);} function div(a, b, k) {var result = a / b; return k(result);} function arithmetic() {add(4, 4, function (a) {sub(a, 2, function (b) {mul(b, 3, function (c) {div(c, 2, function (d) {console.log(d);});});});});} > +// @ sourceMappingURL=code_math.map I suspect it's wrong to modify any file that has a source map. ::: devtools/client/debugger/test/mochitest/code_math_bogus_map.js:4 (Diff revision 1) > /* Any copyright is dedicated to the Public Domain. > http://creativecommons.org/publicdomain/zero/1.0/ */ > -function stopMe(){throw Error("boom");}try{stopMe();var a=1;a=a*2;}catch(e){}; > -//# sourceMappingURL=bogus.map > +function stopMe() {throw Error("boom");} try {stopMe(); var a = 1; a = a * 2;} catch (e) {} > +// # sourceMappingURL=bogus.map Source map. ::: devtools/client/debugger/test/mochitest/code_ugly-4.js:2 (Diff revision 1) > -function a(){b()}function b(){debugger} > -//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiYWJjLmpzIiwic291cmNlcyI6WyJkYXRhOnRleHQvamF2YXNjcmlwdCxmdW5jdGlvbiBhKCl7YigpfSIsImRhdGE6dGV4dC9qYXZhc2NyaXB0LGZ1bmN0aW9uIGIoKXtkZWJ1Z2dlcn0iXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6IkFBQUEsaUJDQUEsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMifQ== > +function a() {b();} function b() {debugger;} > +// # sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiYWJjLmpzIiwic291cmNlcyI6WyJkYXRhOnRleHQvamF2YXNjcmlwdCxmdW5jdGlvbiBhKCl7YigpfSIsImRhdGE6dGV4dC9qYXZhc2NyaXB0LGZ1bmN0aW9uIGIoKXtkZWJ1Z2dlcn0iXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6IkFBQUEsaUJDQUEsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMifQ== Source map. ::: devtools/client/debugger/test/mochitest/code_worker-source-map.js:2 (Diff revision 1) > // Generated by CoffeeScript 1.10.0 > -(function() { > +(function () { Generated code. ::: devtools/client/framework/test/code_ugly.js:1 (Diff revision 1) > -function foo() { var a=1; var b=2; bar(a, b); } > +function foo() { var a = 1; var b = 2; bar(a, b); } Maybe none of the files with "ugly" in their name should be touched. ::: devtools/client/jsonview/lib/require.js:3 (Diff revision 1) > /** vim: et:ts=4:sw=4:sts=4 > * @license RequireJS 2.1.15 Copyright (c) 2010-2014, The Dojo Foundation All Rights Reserved. > * Available via the MIT or new BSD license. Maybe an imported file so shouldn't be touched? ::: devtools/client/shared/demangle.js:6 (Diff revision 1) > /** > * Exposes a function that demangles function names. > * Can be found at: https://github.com/kripken/cxx_demangle > */ > -var demangle = (function() { > +var demangle = (function () { > // In Firefox CommonJS environment, the module boilerplate thinks it's node, As mentioned on irc, a generated file. ::: devtools/server/actors/utils/automation-timeline.js:7 (Diff revision 1) > * web-audio-automation-timeline - 1.0.3 > * https://github.com/jsantell/web-audio-automation-timeline > * MIT License, copyright (c) 2014 Jordan Santell > */ > -!function(e){if("object"==typeof exports&&"undefined"!=typeof module)module.exports=e();else if("function"==typeof define&&define.amd)define([],e);else{var f;"undefined"!=typeof window?f=window:"undefined"!=typeof global?f=global:"undefined"!=typeof self&&(f=self),f.Timeline=e()}}(function(){var define,module,exports;return (function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){ > +!function (e) {if ("object" == typeof exports && "undefined" != typeof module)module.exports = e(); else if ("function" == typeof define && define.amd)define([], e); else {var f; "undefined" != typeof window ? f = window : "undefined" != typeof global ? f = global : "undefined" != typeof self && (f = self), f.Timeline = e();}}(function () {var define, module, exports; return (function e(t, n, r) {function s(o, u) {if (!n[o]) {if (!t[o]) {var a = typeof require == "function" && require; if (!u && a) return a(o, !0); if (i) return i(o, !0); var f = new Error("Cannot find module '" + o + "'"); throw f.code = "MODULE_NOT_FOUND", f;} var l = n[o] = {exports:{}}; t[o][0].call(l.exports, function (e) {var n = t[o][1][e]; return s(n ? n : e);}, l, l.exports, e, t, n, r);} return n[o].exports;} var i = typeof require == "function" && require; for (var o = 0; o < r.length; o++)s(r[o]); return s;})({1:[function (require, module, exports) { > -module.exports = require("./lib/timeline").Timeline; > + module.exports = require("./lib/timeline").Timeline; Not clear if this is a file we should be modifying. ::: devtools/server/tests/unit/babel_and_browserify_script_with_source_map.js:1 (Diff revision 1) > -(function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){ > +(function e(t, n, r) {function s(o, u) {if (!n[o]) {if (!t[o]) {var a = typeof require == "function" && require; if (!u && a) return a(o, !0); if (i) return i(o, !0); var f = new Error("Cannot find module '" + o + "'"); throw f.code = "MODULE_NOT_FOUND", f;} var l = n[o] = {exports:{}}; t[o][0].call(l.exports, function (e) {var n = t[o][1][e]; return s(n ? n : e);}, l, l.exports, e, t, n, r);} return n[o].exports;} var i = typeof require == "function" && require; for (var o = 0; o < r.length; o++)s(r[o]); return s;})({1:[function (require, module, exports) { Probably shouldn't touch this file.
Attachment #8753458 -
Flags: review?(ttromey) → review+
Assignee | ||
Comment 17•8 years ago
|
||
https://reviewboard.mozilla.org/r/53280/#review50096 > I suspect it's wrong to modify any file that has a source map. Thanks for noticing these! I searched the set of changed files to find any that contain `sourceMappingURL` (anywhere in the file) and examined each one. I'll add the bad ones to an explicit ignore list. > Maybe an imported file so shouldn't be touched? Reverted, added to the ignore list. > As mentioned on irc, a generated file. Reverted, added to the ignore list. > Not clear if this is a file we should be modifying. Reverted, added to ignore list.
Assignee | ||
Comment 18•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53356/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53356/
Attachment #8753577 -
Flags: review?(ttromey)
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8753458 [details] MozReview Request: Bug 1271084 - Apply ESLint autofixes to ignored /devtools files. r=tromey Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53280/diff/1-2/
Assignee | ||
Comment 20•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1807c3fcab16
Comment 21•8 years ago
|
||
Comment on attachment 8753577 [details] MozReview Request: Bug 1271084 - Explicitly ignore files with specific style. r=tromey https://reviewboard.mozilla.org/r/53356/#review50282 Thank you. Looks great!
Attachment #8753577 -
Flags: review?(ttromey) → review+
Comment 22•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d86d541ce0f3 https://hg.mozilla.org/integration/fx-team/rev/c4b1eb619c41 https://hg.mozilla.org/integration/fx-team/rev/9661652c2140
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d86d541ce0f3 https://hg.mozilla.org/mozilla-central/rev/c4b1eb619c41 https://hg.mozilla.org/mozilla-central/rev/9661652c2140
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 24•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/8755f5797012
status-firefox48:
--- → fixed
Updated•8 years ago
|
status-firefox48:
fixed → ---
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•