Repository: jlongster/prettier
Pull request #331 (2017-01-19 19:34:25)

Print dangling comments in blocks
I'm still thinking through the best way to handle comments. For now, I'm going to fix a few smaller bugs first so I can get a better understanding of all the edge cases. This one fixes the empty block problem.

Once I see look at more use cases and how the AST is structured I'll see if there's a better way to handle comments generally.

vjeux: Looks good to me!

Patch:
diff --git a/src/comments.js b/src/comments.js
index 0283f3a0e..20166ae25 100644
--- a/src/comments.js
+++ b/src/comments.js
@@ -268,6 +268,21 @@ function printTrailingComment(commentPath, print, options) {
   ]);
 }
 
+function printDanglingComments(path, print, options) {
+  const text = options.originalText;
+
+  const parts = [];
+  path.each(commentPath => {
+    const comment = commentPath.getValue();
+    if(!comment.leading && !comment.trailing) {
+      parts.push(util.newlineExistsBefore(text, locStart(comment)) ? hardline : " ");
+      parts.push(commentPath.call(print));
+    }
+  }, "comments");
+  return concat(parts);
+}
+exports.printDanglingComments = printDanglingComments;
+
 exports.printComments = function(path, print, options) {
   var value = path.getValue();
   var parent = path.getParentNode();
diff --git a/src/printer.js b/src/printer.js
index 3291a7207..26ad38103 100644
--- a/src/printer.js
+++ b/src/printer.js
@@ -1,6 +1,6 @@
 "use strict";
 var assert = require("assert");
-var printComments = require("./comments").printComments;
+var comments = require("./comments");
 var pp = require("./pp");
 var fromString = pp.fromString;
 var concat = pp.concat;
@@ -36,7 +36,7 @@ function Printer(originalOptions) {
 
   // Print the entire AST generically.
   function printGenerically(path) {
-    return printComments(
+    return comments.printComments(
       path,
       p => genericPrint(p, options, printGenerically),
       options
@@ -510,6 +510,14 @@ function genericPrintNoParens(path, options, print) {
       if (hasContent) {
         parts.push(indent(options.tabWidth, concat([ hardline, naked ])));
       }
+      else if(n.comments) {
+        parts.push(
+          indent(
+            options.tabWidth,
+            comments.printDanglingComments(path, print, options)
+          )
+        );
+      }
 
       parts.push(hardline, "}");
 


Test patch:
diff --git a/tests/comments/__snapshots__/jsfmt.spec.js.snap b/tests/comments/__snapshots__/jsfmt.spec.js.snap
index 19488fcb1..bb706efb6 100644
--- a/tests/comments/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/comments/__snapshots__/jsfmt.spec.js.snap
@@ -179,6 +179,7 @@ Observable
 
 // Comments disappear in empty blocks
 if (1) {
+  // Comment
 }
 
 // Comments trigger invalid JavaScript in-between else if
@@ -374,6 +375,7 @@ div />;
 
 // Comments disappear in empty blocks
 if (1) {
+  // Comment
 }
 
 // Comments trigger invalid JavaScript in-between else if
diff --git a/tests/iter/__snapshots__/jsfmt.spec.js.snap b/tests/iter/__snapshots__/jsfmt.spec.js.snap
index bfd088d91..6cad6006d 100644
--- a/tests/iter/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/iter/__snapshots__/jsfmt.spec.js.snap
@@ -72,6 +72,8 @@ for (var x in null) {
 }
 
 for (var y in this) {
+  // regression test to make sure \`in this\` doesn\'t fatal. it\'s currently
+  // allowed, even though we can\'t actually enumerate all the keys on \`this\`.
 }
 "
 `;
diff --git a/tests/more_path/__snapshots__/jsfmt.spec.js.snap b/tests/more_path/__snapshots__/jsfmt.spec.js.snap
index 2edb50eb4..8c8ab40d8 100644
--- a/tests/more_path/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/more_path/__snapshots__/jsfmt.spec.js.snap
@@ -102,7 +102,8 @@ function goofy() {
   var x = g();
   if (typeof x == \"function\") {
     x();
-  } else {
+  } else { // if (typeof x == \'number\') {
+    //f(x);
   }
 }
 
@@ -115,6 +116,7 @@ function goofy2() {
   if (typeof y == \"function\") {
     y();
   } else {
+    //f(y);
   }
 }
 
@@ -238,6 +240,7 @@ class D {
 
 function baz(x: D) {
   if (x instanceof A) {
+    // unreachable, TODO: this shouldn\'t throw
   }
 }
 
diff --git a/tests/refinements/__snapshots__/jsfmt.spec.js.snap b/tests/refinements/__snapshots__/jsfmt.spec.js.snap
index 49eddc5a9..249a95d4d 100644
--- a/tests/refinements/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/refinements/__snapshots__/jsfmt.spec.js.snap
@@ -1414,11 +1414,11 @@ let tests = [
     }
   },
   function(num: number, obj: { foo: number }) {
-    if (num === obj.bar) {
+    if (num === obj.bar) { // ok, typos allowed in conditionals
     }
   },
   function(num: number, obj: { [key: string]: number }) {
-    if (num === obj.bar) {
+    if (num === obj.bar) { // ok
     }
   },
   function(n: number): Mode {
@@ -1994,11 +1994,11 @@ let tests = [
     }
   },
   function(str: string, obj: { foo: string }) {
-    if (str === obj.bar) {
+    if (str === obj.bar) { // ok, typos allowed in conditionals
     }
   },
   function(str: string, obj: { [key: string]: string }) {
-    if (str === obj.bar) {
+    if (str === obj.bar) { // ok
     }
   },
   function(str: string): Mode {
diff --git a/tests/template/__snapshots__/jsfmt.spec.js.snap b/tests/template/__snapshots__/jsfmt.spec.js.snap
index 8025ccee6..df29387a9 100644
--- a/tests/template/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/template/__snapshots__/jsfmt.spec.js.snap
@@ -16,6 +16,7 @@ var x = {
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 var o = {
   [\`key\`]: () => {
+    // Comment
   }
 };
 
diff --git a/tests/try/__snapshots__/jsfmt.spec.js.snap b/tests/try/__snapshots__/jsfmt.spec.js.snap
index 93a7bbae1..b3c1223d7 100644
--- a/tests/try/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/try/__snapshots__/jsfmt.spec.js.snap
@@ -561,7 +561,8 @@ function bar(response) {
     throw new Error(\"...\");
   }
   // here via [try] only.
-  if (payload.error) {
+  if (payload.error) { // ok
+    // ...
   }
 }
 
diff --git a/tests/typecast/__snapshots__/jsfmt.spec.js.snap b/tests/typecast/__snapshots__/jsfmt.spec.js.snap
index 802943aa2..fa443d21b 100644
--- a/tests/typecast/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/typecast/__snapshots__/jsfmt.spec.js.snap
@@ -43,6 +43,11 @@ var tests = [
     var s: string = ((0: number), (\"hey\": string));
   },
   () => {
+    // TODO pending array element inference issues
+    // control case:
+    // var a : Array<?number> = [0, 1, 2, 3, 4, 5, 6, 7, null];
+    // typecast case:
+    // var b = [(0 : ?number), 1, 2, 3, 4, 5, 6, 7, null];
   }
 ];
 "