From 9eb389b9f4580907fe0359eea83b4b2e7757c023 Mon Sep 17 00:00:00 2001 From: Royce Townsend Date: Sun, 19 Mar 2017 02:20:07 +0800 Subject: [PATCH] Group first arg for inline functions (#947) * Group first arg for inline functions * Group first arg. Unless there are comments. * Group first arg. Allow second arg to be empty object/array. This implementation has a side effect of disallowing empty objects/arrays in the should group last arg heuristic. --- src/printer.js | 77 +++++-- .../__snapshots__/jsfmt.spec.js.snap | 200 ++++++++++++++++++ tests/first_argument_expansion/jsfmt.spec.js | 1 + tests/first_argument_expansion/test.js | 87 ++++++++ .../arraylib/__snapshots__/jsfmt.spec.js.snap | 9 +- .../__snapshots__/jsfmt.spec.js.snap | 9 +- .../__snapshots__/jsfmt.spec.js.snap | 102 +++++++++ tests/last_argument_expansion/overflow.js | 16 ++ 8 files changed, 466 insertions(+), 35 deletions(-) create mode 100644 tests/first_argument_expansion/__snapshots__/jsfmt.spec.js.snap create mode 100644 tests/first_argument_expansion/jsfmt.spec.js create mode 100644 tests/first_argument_expansion/test.js diff --git a/src/printer.js b/src/printer.js index 42f993fdb..2e62b9f5f 100644 --- a/src/printer.js +++ b/src/printer.js @@ -1925,25 +1925,43 @@ function printMethod(path, options, print) { return concat(parts); } +function couldGroupArg(arg) { + return ((arg.type === "ObjectExpression" && arg.properties.length > 0) || + (arg.type === "ArrayExpression" && arg.elements.length > 0) || + arg.type === "FunctionExpression" || + (arg.type === "ArrowFunctionExpression" && + (arg.body.type === "BlockStatement" || + arg.body.type === "ArrowFunctionExpression" || + arg.body.type === "ObjectExpression" || + arg.body.type === "ArrayExpression" || + arg.body.type === "CallExpression" || + arg.body.type === "JSXElement"))); +} + function shouldGroupLastArg(args) { const lastArg = util.getLast(args); const penultimateArg = util.getPenultimate(args); return (!lastArg.comments || !lastArg.comments.length) && - (lastArg.type === "ObjectExpression" || - lastArg.type === "ArrayExpression" || - lastArg.type === "FunctionExpression" || - (lastArg.type === "ArrowFunctionExpression" && - (lastArg.body.type === "BlockStatement" || - lastArg.body.type === "ArrowFunctionExpression" || - lastArg.body.type === "ObjectExpression" || - lastArg.body.type === "ArrayExpression" || - lastArg.body.type === "CallExpression" || - lastArg.body.type === "JSXElement"))) && + couldGroupArg(lastArg) && // If the last two arguments are of the same type, // disable last element expansion. (!penultimateArg || penultimateArg.type !== lastArg.type); } +function shouldGroupFirstArg(args) { + if (args.length !== 2) { + return false; + } + + const firstArg = args[0]; + const secondArg = args[1]; + return (!firstArg.comments || !firstArg.comments.length) && + (firstArg.type === 'FunctionExpression' || + (firstArg.type === 'ArrowFunctionExpression' && + firstArg.body.type === 'BlockStatement')) && + !couldGroupArg(secondArg); +} + function printArgumentsList(path, options, print) { var printed = path.map(print, "arguments"); @@ -1959,20 +1977,31 @@ function printArgumentsList(path, options, print) { // This is just an optimization; I think we could return the // conditional group for all function calls, but it's more expensive // so only do it for specific forms. - if (shouldGroupLastArg(args)) { - const shouldBreak = printed.slice(0, -1).some(willBreak); + const shouldGroupFirst = shouldGroupFirstArg(args); + if (shouldGroupFirst || shouldGroupLastArg(args)) { + const shouldBreak = shouldGroupFirst + ? printed.slice(1).some(willBreak) + : printed.slice(0, -1).some(willBreak); return concat([ printed.some(willBreak) ? breakParent : "", conditionalGroup( [ concat(["(", join(concat([", "]), printed), ")"]), - concat([ - "(", - join(concat([",", line]), printed.slice(0, -1)), - printed.length > 1 ? ", " : "", - group(util.getLast(printed), { shouldBreak: true }), - ")" - ]), + shouldGroupFirst + ? concat([ + "(", + group(printed[0], { shouldBreak: true }), + printed.length > 1 ? ", " : "", + join(concat([",", line]), printed.slice(1)), + ")" + ]) + : concat([ + "(", + join(concat([",", line]), printed.slice(0, -1)), + printed.length > 1 ? ", " : "", + group(util.getLast(printed), { shouldBreak: true }), + ")" + ]), group( concat([ "(", @@ -2042,8 +2071,8 @@ function printFunctionParams(path, print, options) { const canHaveTrailingComma = !(lastParam && lastParam.type === "RestElement") && !fun.rest; - // If the parent is a call with the last argument expansion and this is the - // params of the last argument, we dont want the arguments to break and instead + // If the parent is a call with the first/last argument expansion and this is the + // params of the first/last argument, we dont want the arguments to break and instead // want the whole expression to be on a new line. // // Good: Bad: @@ -2055,8 +2084,10 @@ function printFunctionParams(path, print, options) { const parent = path.getParentNode(); if ( (parent.type === "CallExpression" || parent.type === "NewExpression") && - util.getLast(parent.arguments) === path.getValue() && - shouldGroupLastArg(parent.arguments) + ((util.getLast(parent.arguments) === path.getValue() && + shouldGroupLastArg(parent.arguments)) || + (parent.arguments[0] === path.getValue() && + shouldGroupFirstArg(parent.arguments))) ) { return concat(["(", join(", ", printed), ")"]); } diff --git a/tests/first_argument_expansion/__snapshots__/jsfmt.spec.js.snap b/tests/first_argument_expansion/__snapshots__/jsfmt.spec.js.snap new file mode 100644 index 000000000..6b86606bb --- /dev/null +++ b/tests/first_argument_expansion/__snapshots__/jsfmt.spec.js.snap @@ -0,0 +1,200 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`test.js 1`] = ` +"setTimeout(function() { + thing(); +}, 500); + +[\\"a\\",\\"b\\",\\"c\\"].reduce(function(item, thing) { + return thing + \\" \\" + item; +}, \\"letters:\\") + +func(() => { + thing(); +}, identifier); + +func(function() { + thing(); +}, this.props.timeout * 1000); + +func((that) => { + thing(); +}, this.props.getTimeout()); + +func(() => { + thing(); +}, true); + +func(() => { + thing(); +}, null); + +func(() => { + thing(); +}, undefined); + +func(() => { + thing(); +}, /regex.*?/); + +compose((a) => { + return a.thing; +}, b => b * b); + +somthing.reduce(function(item, thing) { + return thing.blah = item; +}, {}) + +somthing.reduce(function(item, thing) { + return thing.push(item); +}, []) + +reallyLongLongLongLongLongLongLongLongLongLongLongLongLongLongMethod((f, g, h) => { + return f.pop(); +}, true); + +// Don't do the rest of these + +func(function() { + thing(); +}, true, false); + +func(() => { + thing(); +}, {yes: true, cats: 5}); + +compose((a) => { + return a.thing; +}, b => { + return b + \\"\\"; +}); + +compose((a) => { + return a.thing; +}, b => [1, 2, 3, 4, 5]); + +renderThing(a => +
Content. So much to say. Oh my. Are we done yet?
+,args); + +setTimeout( + // Something + function() { + thing(); + }, + 500 +); + +setTimeout(/* blip */ function() { + thing(); +}, 500); +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +setTimeout(function() { + thing(); +}, 500); + +[\\"a\\", \\"b\\", \\"c\\"].reduce(function(item, thing) { + return thing + \\" \\" + item; +}, \\"letters:\\"); + +func(() => { + thing(); +}, identifier); + +func(function() { + thing(); +}, this.props.timeout * 1000); + +func(that => { + thing(); +}, this.props.getTimeout()); + +func(() => { + thing(); +}, true); + +func(() => { + thing(); +}, null); + +func(() => { + thing(); +}, undefined); + +func(() => { + thing(); +}, /regex.*?/); + +compose(a => { + return a.thing; +}, b => b * b); + +somthing.reduce(function(item, thing) { + return (thing.blah = item); +}, {}); + +somthing.reduce(function(item, thing) { + return thing.push(item); +}, []); + +reallyLongLongLongLongLongLongLongLongLongLongLongLongLongLongMethod( + (f, g, h) => { + return f.pop(); + }, + true +); + +// Don't do the rest of these + +func( + function() { + thing(); + }, + true, + false +); + +func( + () => { + thing(); + }, + { yes: true, cats: 5 } +); + +compose( + a => { + return a.thing; + }, + b => { + return b + \\"\\"; + } +); + +compose( + a => { + return a.thing; + }, + b => [1, 2, 3, 4, 5] +); + +renderThing( + a =>
Content. So much to say. Oh my. Are we done yet?
, + args +); + +setTimeout( + // Something + function() { + thing(); + }, + 500 +); + +setTimeout( + /* blip */ function() { + thing(); + }, + 500 +); +" +`; diff --git a/tests/first_argument_expansion/jsfmt.spec.js b/tests/first_argument_expansion/jsfmt.spec.js new file mode 100644 index 000000000..989047bcc --- /dev/null +++ b/tests/first_argument_expansion/jsfmt.spec.js @@ -0,0 +1 @@ +run_spec(__dirname); diff --git a/tests/first_argument_expansion/test.js b/tests/first_argument_expansion/test.js new file mode 100644 index 000000000..a27a3cdcf --- /dev/null +++ b/tests/first_argument_expansion/test.js @@ -0,0 +1,87 @@ +setTimeout(function() { + thing(); +}, 500); + +["a","b","c"].reduce(function(item, thing) { + return thing + " " + item; +}, "letters:") + +func(() => { + thing(); +}, identifier); + +func(function() { + thing(); +}, this.props.timeout * 1000); + +func((that) => { + thing(); +}, this.props.getTimeout()); + +func(() => { + thing(); +}, true); + +func(() => { + thing(); +}, null); + +func(() => { + thing(); +}, undefined); + +func(() => { + thing(); +}, /regex.*?/); + +compose((a) => { + return a.thing; +}, b => b * b); + +somthing.reduce(function(item, thing) { + return thing.blah = item; +}, {}) + +somthing.reduce(function(item, thing) { + return thing.push(item); +}, []) + +reallyLongLongLongLongLongLongLongLongLongLongLongLongLongLongMethod((f, g, h) => { + return f.pop(); +}, true); + +// Don't do the rest of these + +func(function() { + thing(); +}, true, false); + +func(() => { + thing(); +}, {yes: true, cats: 5}); + +compose((a) => { + return a.thing; +}, b => { + return b + ""; +}); + +compose((a) => { + return a.thing; +}, b => [1, 2, 3, 4, 5]); + +renderThing(a => +
Content. So much to say. Oh my. Are we done yet?
+,args); + +setTimeout( + // Something + function() { + thing(); + }, + 500 +); + +setTimeout(/* blip */ function() { + thing(); +}, 500); diff --git a/tests/flow/arraylib/__snapshots__/jsfmt.spec.js.snap b/tests/flow/arraylib/__snapshots__/jsfmt.spec.js.snap index f573a5085..c6ec7c02a 100644 --- a/tests/flow/arraylib/__snapshots__/jsfmt.spec.js.snap +++ b/tests/flow/arraylib/__snapshots__/jsfmt.spec.js.snap @@ -94,12 +94,9 @@ function reduce_test() { return previousValue + currentValue + array[index]; }); - [0, 1, 2, 3, 4].reduce( - function(previousValue, currentValue, index, array) { - return previousValue + currentValue + array[index]; - }, - 10 - ); + [0, 1, 2, 3, 4].reduce(function(previousValue, currentValue, index, array) { + return previousValue + currentValue + array[index]; + }, 10); var total = [0, 1, 2, 3].reduce(function(a, b) { return a + b; diff --git a/tests/flow/union_new/__snapshots__/jsfmt.spec.js.snap b/tests/flow/union_new/__snapshots__/jsfmt.spec.js.snap index 72de80d5a..00de0123a 100644 --- a/tests/flow/union_new/__snapshots__/jsfmt.spec.js.snap +++ b/tests/flow/union_new/__snapshots__/jsfmt.spec.js.snap @@ -1597,12 +1597,9 @@ exports[`test20.js 1`] = ` [0, 1].reduce((x, y, i) => y); -[\\"a\\", \\"b\\"].reduce( - (regex, representation, index) => { - return regex + (index ? \\"|\\" : \\"\\") + \\"(\\" + representation + \\")\\"; - }, - \\"\\" -); +[\\"a\\", \\"b\\"].reduce((regex, representation, index) => { + return regex + (index ? \\"|\\" : \\"\\") + \\"(\\" + representation + \\")\\"; +}, \\"\\"); [\\"\\"].reduce((acc, str) => acc * str.length); " diff --git a/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap b/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap index 56240daef..a814bab1d 100644 --- a/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap +++ b/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap @@ -154,11 +154,113 @@ exports[`overflow.js 1`] = ` "SuperSuperSuperSuperSuperSuperSuperSuperSuperSuperSuperSuperLongCall((err, result) => { // comment }); + +func(one, two, three, four, five, six, seven, eig, is, this, too, long, no, []); +func(one, two, three, four, five, six, seven, eig, is, this, too, long, yes, []); +func(one, two, three, four, five, six, seven, eig, is, this, too, long, yes, [ + // Comments +]); +func(five, six, seven, eig, is, this, too, long, yes, [ + // Comments +]); + +func(one, two, three, four, five, six, seven, eig, is, this, too, long, no, {}); +func(one, two, three, four, five, six, seven, eig, is, this, too, long, yes, {}); +func(one, two, three, four, five, six, seven, eig, is, this, too, long, yes, { + // Comments +}); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ SuperSuperSuperSuperSuperSuperSuperSuperSuperSuperSuperSuperLongCall( (err, result) => { // comment } ); + +func(one, two, three, four, five, six, seven, eig, is, this, too, long, no, []); +func( + one, + two, + three, + four, + five, + six, + seven, + eig, + is, + this, + too, + long, + yes, + [] +); +func( + one, + two, + three, + four, + five, + six, + seven, + eig, + is, + this, + too, + long, + yes, + [ + // Comments + ] +); +func( + five, + six, + seven, + eig, + is, + this, + too, + long, + yes, + [ + // Comments + ] +); + +func(one, two, three, four, five, six, seven, eig, is, this, too, long, no, {}); +func( + one, + two, + three, + four, + five, + six, + seven, + eig, + is, + this, + too, + long, + yes, + {} +); +func( + one, + two, + three, + four, + five, + six, + seven, + eig, + is, + this, + too, + long, + yes, + { + // Comments + } +); " `; diff --git a/tests/last_argument_expansion/overflow.js b/tests/last_argument_expansion/overflow.js index 1c97a0e6f..0f0c20319 100644 --- a/tests/last_argument_expansion/overflow.js +++ b/tests/last_argument_expansion/overflow.js @@ -1,3 +1,19 @@ SuperSuperSuperSuperSuperSuperSuperSuperSuperSuperSuperSuperLongCall((err, result) => { // comment }); + +func(one, two, three, four, five, six, seven, eig, is, this, too, long, no, []); +func(one, two, three, four, five, six, seven, eig, is, this, too, long, yes, []); +func(one, two, three, four, five, six, seven, eig, is, this, too, long, yes, [ + // Comments +]); +func(five, six, seven, eig, is, this, too, long, yes, [ + // Comments +]); + +func(one, two, three, four, five, six, seven, eig, is, this, too, long, no, {}); +func(one, two, three, four, five, six, seven, eig, is, this, too, long, yes, {}); +func(one, two, three, four, five, six, seven, eig, is, this, too, long, yes, { + // Comments +}); +