Run ./mach eslint devtools/ --fix --no-ignore

RESOLVED FIXED in Firefox 49

Status

enhancement
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: ntim, Assigned: jryans)

Tracking

(Blocks 1 bug)

unspecified
Firefox 49
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

3 years ago
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

3 years ago
Reporter

Updated

3 years ago
Flags: needinfo?(pbrosset)
Reporter

Comment 1

3 years ago
I use the --fix parameter systematically because it saves lots of time, and doesn't cause problems.
Reporter

Comment 2

3 years ago
I had to split the patch in 3 parts, because the original was too big for bugzilla.
Reporter

Comment 3

3 years ago
Reporter

Comment 4

3 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)
(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

3 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

3 years ago
Severity: normal → enhancement
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

3 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

3 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

3 years ago
Let's see if MozReview can deal with one giant combined patch...
Assignee

Updated

3 years ago
Attachment #8750037 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8750038 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8750039 - Attachment is obsolete: true
Assignee

Comment 13

3 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/
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 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

3 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 19

3 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/
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 23

3 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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee

Updated

3 years ago
Depends on: 1274333
Reporter

Updated

3 years ago
Blocks: 1256789
Reporter

Updated

3 years ago
Blocks: 1256822
Reporter

Updated

3 years ago
Blocks: 1256777
Reporter

Updated

3 years ago
Blocks: 1256817
Assignee

Updated

3 years ago
Depends on: 1278357
Assignee

Updated

3 years ago
Depends on: 1278413

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.