Merge pull request #3324 from suchipi/arrow_parens

Simple arrow function parens option
prettier · Nov 28, 2017 · 606c5ab · 606c5ab
2 parents a02e3b3 + 41a9aba
commit 606c5ab
Show file tree

Hide file tree

Showing 15 changed files with 1,001 additions and 39 deletions.
diff --git a/docs/options.md b/docs/options.md
@@ -96,6 +96,19 @@ Put the `>` of a multi-line JSX element at the end of the last line instead of b
 | ------- | ------------------------- | ---------------------------- |
 | `false` | `--jsx-bracket-same-line` | `jsxBracketSameLine: <bool>` |
 
+## Arrow Function Parentheses
+
+Include parentheses around a sole arrow function parameter.
+
+Valid options:
+
+* `"avoid"` - Omit parens when possible. Example: `x => x`
+* `"always"` - Always include parens. Example: `(x) => x`
+
+| Default   | CLI Override                                    | API Override                                    |
+| --------- | ----------------------------------------------- | ----------------------------------------------- |
+| `"avoid"` | <code>--arrow-parens <avoid&#124;always></code> | <code>arrowParens: "<avoid&#124;always>"</code> |
+
 ## Range
 
 Format only a segment of a file.

diff --git a/src/cli-constant.js b/src/cli-constant.js
@@ -77,6 +77,23 @@ const categoryOrder = [
  * Note: The options below are sorted alphabetically.
  */
 const detailedOptions = normalizeDetailedOptions({
+  "arrow-parens": {
+    type: "choice",
+    category: CATEGORY_FORMAT,
+    forwardToApi: true,
+    description: "Include parentheses around a sole arrow function parameter.",
+    default: "avoid",
+    choices: [
+      {
+        value: "avoid",
+        description: "Omit parens when possible. Example: `x => x`"
+      },
+      {
+        value: "always",
+        description: "Always include parens. Example: `(x) => x`"
+      }
+    ]
+  },
   "bracket-spacing": {
     type: "boolean",
     category: CATEGORY_FORMAT,

diff --git a/src/options.js b/src/options.js
@@ -21,7 +21,8 @@ const defaults = {
   insertPragma: false,
   requirePragma: false,
   semi: true,
-  proseWrap: true
+  proseWrap: true,
+  arrowParens: "avoid"
 };
 
 const exampleConfig = Object.assign({}, defaults, {

diff --git a/src/printer.js b/src/printer.js
@@ -214,14 +214,6 @@ function genericPrint(path, options, printPath, args) {
     needsParens = path.needsParens(options);
   }
 
-  if (node.type) {
-    // HACK: ASI prevention in no-semi mode relies on knowledge of whether
-    // or not a paren has been inserted (see `exprNeedsASIProtection()`).
-    // For now, we're just passing that information by mutating the AST here,
-    // but it would be nice to find a cleaner way to do this.
-    node.needsParens = needsParens;
-  }
-
   const parts = [];
   if (needsParens) {
     parts.unshift("(");
@@ -501,7 +493,7 @@ function genericPrintNoParens(path, options, print, args) {
         parts.push("async ");
       }
 
-      if (canPrintParamsWithoutParens(n)) {
+      if (shouldPrintParamsWithoutParens(path, options)) {
         parts.push(path.call(print, "params", 0));
       } else {
         parts.push(
@@ -855,7 +847,6 @@ function genericPrintNoParens(path, options, print, args) {
     case "NewExpression":
     case "CallExpression": {
       const isNew = n.type === "NewExpression";
-      const unitTestRe = /^(f|x)?(it|describe|test)$/;
 
       const optional = printOptionalToken(path);
       if (
@@ -869,22 +860,7 @@ function genericPrintNoParens(path, options, print, args) {
           isTemplateOnItsOwnLine(n.arguments[0], options.originalText)) ||
         // Keep test declarations on a single line
         // e.g. `it('long name', () => {`
-        (!isNew &&
-          ((n.callee.type === "Identifier" && unitTestRe.test(n.callee.name)) ||
-            (n.callee.type === "MemberExpression" &&
-              n.callee.object.type === "Identifier" &&
-              n.callee.property.type === "Identifier" &&
-              unitTestRe.test(n.callee.object.name) &&
-              (n.callee.property.name === "only" ||
-                n.callee.property.name === "skip"))) &&
-          n.arguments.length === 2 &&
-          (n.arguments[0].type === "StringLiteral" ||
-            n.arguments[0].type === "TemplateLiteral" ||
-            (n.arguments[0].type === "Literal" &&
-              typeof n.arguments[0].value === "string")) &&
-          (n.arguments[1].type === "FunctionExpression" ||
-            n.arguments[1].type === "ArrowFunctionExpression") &&
-          n.arguments[1].params.length <= 1)
+        (!isNew && isTestCall(n))
       ) {
         return concat([
           isNew ? "new " : "",
@@ -2873,11 +2849,9 @@ function printStatementSequence(path, options, print) {
       !options.semi &&
       !isClass &&
       !isTheOnlyJSXElementInMarkdown(options, stmtPath) &&
-      stmtNeedsASIProtection(stmtPath)
+      stmtNeedsASIProtection(stmtPath, options)
     ) {
       if (stmt.comments && stmt.comments.some(comment => comment.leading)) {
-        // Note: stmtNeedsASIProtection requires stmtPath to already be printed
-        // as it reads needsParens which is mutated on the instance
         parts.push(print(stmtPath, { needsSemi: true }));
       } else {
         parts.push(";", stmtPrinted);
@@ -3228,6 +3202,11 @@ function printFunctionParams(path, print, options, expandArg, printTypeParams) {
 
   const parent = path.getParentNode();
 
+  // don't break in specs, eg; `it("should maintain parens around done even when long", (done) => {})`
+  if (parent.type === "CallExpression" && isTestCall(parent)) {
+    return concat([typeParams, "(", join(", ", printed), ")"]);
+  }
+
   const flowTypeAnnotations = [
     "AnyTypeAnnotation",
     "NullLiteralTypeAnnotation",
@@ -3282,6 +3261,20 @@ function printFunctionParams(path, print, options, expandArg, printTypeParams) {
   ]);
 }
 
+function shouldPrintParamsWithoutParens(path, options) {
+  if (options.arrowParens === "always") {
+    return false;
+  }
+
+  if (options.arrowParens === "avoid") {
+    const node = path.getValue();
+    return canPrintParamsWithoutParens(node);
+  }
+
+  // Fallback default; should be unreachable
+  return false;
+}
+
 function canPrintParamsWithoutParens(node) {
   return (
     node.params.length === 1 &&
@@ -3527,7 +3520,15 @@ function printTypeParameters(path, options, print, paramsKey) {
     return path.call(print, paramsKey);
   }
 
+  const grandparent = path.getNode(2);
+
+  const isParameterInTestCall =
+    grandparent != null &&
+    grandparent.type === "CallExpression" &&
+    isTestCall(grandparent);
+
   const shouldInline =
+    isParameterInTestCall ||
     n[paramsKey].length === 0 ||
     (n[paramsKey].length === 1 &&
       (shouldHugType(n[paramsKey][0]) ||
@@ -4683,15 +4684,43 @@ function getLeftSide(node) {
   );
 }
 
-function exprNeedsASIProtection(node) {
-  // HACK: node.needsParens is added in `genericPrint()` for the sole purpose
-  // of being used here. It'd be preferable to find a cleaner way to do this.
+function getLeftSidePathName(path, node) {
+  if (node.expressions) {
+    return ["expressions", 0];
+  }
+  if (node.left) {
+    return ["left"];
+  }
+  if (node.test) {
+    return ["test"];
+  }
+  if (node.callee) {
+    return ["callee"];
+  }
+  if (node.object) {
+    return ["object"];
+  }
+  if (node.tag) {
+    return ["tag"];
+  }
+  if (node.argument) {
+    return ["argument"];
+  }
+  if (node.expression) {
+    return ["expression"];
+  }
+  throw new Error("Unexpected node has no left side", node);
+}
+
+function exprNeedsASIProtection(path, options) {
+  const node = path.getValue();
+
   const maybeASIProblem =
-    node.needsParens ||
+    path.needsParens(options) ||
     node.type === "ParenthesizedExpression" ||
     node.type === "TypeCastExpression" ||
     (node.type === "ArrowFunctionExpression" &&
-      !canPrintParamsWithoutParens(node)) ||
+      !shouldPrintParamsWithoutParens(path, options)) ||
     node.type === "ArrayExpression" ||
     node.type === "ArrayPattern" ||
     (node.type === "UnaryExpression" &&
@@ -4713,17 +4742,25 @@ function exprNeedsASIProtection(node) {
     return false;
   }
 
-  return exprNeedsASIProtection(getLeftSide(node));
+  return path.call.apply(
+    path,
+    [childPath => exprNeedsASIProtection(childPath, options)].concat(
+      getLeftSidePathName(path, node)
+    )
+  );
 }
 
-function stmtNeedsASIProtection(path) {
+function stmtNeedsASIProtection(path, options) {
   const node = path.getNode();
 
   if (node.type !== "ExpressionStatement") {
     return false;
   }
 
-  return exprNeedsASIProtection(node.expression);
+  return path.call(
+    childPath => exprNeedsASIProtection(childPath, options),
+    "expression"
+  );
 }
 
 function classPropMayCauseASIProblems(path) {
@@ -4992,6 +5029,28 @@ function isObjectType(n) {
   return n.type === "ObjectTypeAnnotation" || n.type === "TSTypeLiteral";
 }
 
+// eg; `describe("some string", (done) => {})`
+function isTestCall(n) {
+  const unitTestRe = /^(f|x)?(it|describe|test)$/;
+  return (
+    ((n.callee.type === "Identifier" && unitTestRe.test(n.callee.name)) ||
+      (n.callee.type === "MemberExpression" &&
+        n.callee.object.type === "Identifier" &&
+        n.callee.property.type === "Identifier" &&
+        unitTestRe.test(n.callee.object.name) &&
+        (n.callee.property.name === "only" ||
+          n.callee.property.name === "skip"))) &&
+    n.arguments.length === 2 &&
+    (n.arguments[0].type === "StringLiteral" ||
+      n.arguments[0].type === "TemplateLiteral" ||
+      (n.arguments[0].type === "Literal" &&
+        typeof n.arguments[0].value === "string")) &&
+    (n.arguments[1].type === "FunctionExpression" ||
+      n.arguments[1].type === "ArrowFunctionExpression") &&
+    n.arguments[1].params.length <= 1
+  );
+}
+
 function printAstToDoc(ast, options, addAlignmentSize) {
   addAlignmentSize = addAlignmentSize || 0;
 

diff --git a/tests/arrow-call/__snapshots__/jsfmt.spec.js.snap b/tests/arrow-call/__snapshots__/jsfmt.spec.js.snap
@@ -189,3 +189,98 @@ const composition = (ViewComponent, ContainerComponent) =>
   };
 
 `;
+
+exports[`arrow_call.js 3`] = `
+const testResults = results.testResults.map(testResult =>
+  formatResult(testResult, formatter, reporter)
+);
+
+it('mocks regexp instances', () => {
+  expect(
+    () => moduleMocker.generateFromMetadata(moduleMocker.getMetadata(/a/)),
+  ).not.toThrow();
+});
+
+expect(() => asyncRequest({ url: "/test-endpoint" }))
+  .toThrowError(/Required parameter/);
+
+expect(() => asyncRequest({ url: "/test-endpoint-but-with-a-long-url" }))
+  .toThrowError(/Required parameter/);
+
+expect(() => asyncRequest({ url: "/test-endpoint-but-with-a-suuuuuuuuper-long-url" }))
+  .toThrowError(/Required parameter/);
+
+expect(() => asyncRequest({ type: "foo", url: "/test-endpoint" }))
+  .not.toThrowError();
+
+expect(() => asyncRequest({ type: "foo", url: "/test-endpoint-but-with-a-long-url" }))
+  .not.toThrowError();
+
+const a = Observable
+  .fromPromise(axiosInstance.post('/carts/mine'))
+  .map((response) => response.data)
+
+const b = Observable.fromPromise(axiosInstance.get(url))
+  .map((response) => response.data)
+
+func(
+  veryLoooooooooooooooooooooooongName,
+  veryLooooooooooooooooooooooooongName =>
+    veryLoooooooooooooooongName.something()
+);
+
+const composition = (ViewComponent, ContainerComponent) =>
+  class extends React.Component {
+    static propTypes = {};
+  };
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+const testResults = results.testResults.map((testResult) =>
+  formatResult(testResult, formatter, reporter)
+);
+
+it("mocks regexp instances", () => {
+  expect(() =>
+    moduleMocker.generateFromMetadata(moduleMocker.getMetadata(/a/))
+  ).not.toThrow();
+});
+
+expect(() => asyncRequest({ url: "/test-endpoint" })).toThrowError(
+  /Required parameter/
+);
+
+expect(() =>
+  asyncRequest({ url: "/test-endpoint-but-with-a-long-url" })
+).toThrowError(/Required parameter/);
+
+expect(() =>
+  asyncRequest({ url: "/test-endpoint-but-with-a-suuuuuuuuper-long-url" })
+).toThrowError(/Required parameter/);
+
+expect(() =>
+  asyncRequest({ type: "foo", url: "/test-endpoint" })
+).not.toThrowError();
+
+expect(() =>
+  asyncRequest({ type: "foo", url: "/test-endpoint-but-with-a-long-url" })
+).not.toThrowError();
+
+const a = Observable.fromPromise(axiosInstance.post("/carts/mine")).map(
+  (response) => response.data
+);
+
+const b = Observable.fromPromise(axiosInstance.get(url)).map(
+  (response) => response.data
+);
+
+func(
+  veryLoooooooooooooooooooooooongName,
+  (veryLooooooooooooooooooooooooongName) =>
+    veryLoooooooooooooooongName.something()
+);
+
+const composition = (ViewComponent, ContainerComponent) =>
+  class extends React.Component {
+    static propTypes = {};
+  };
+
+`;
diff --git a/tests/arrow-call/jsfmt.spec.js b/tests/arrow-call/jsfmt.spec.js
@@ -1,2 +1,3 @@
 run_spec(__dirname, null, ["typescript"]);
 run_spec(__dirname, { trailingComma: "all" }, ["typescript"]);
+run_spec(__dirname, { arrowParens: "always" }, ["typescript"]);