Repository: google/closure-compiler
Pull request #783 (2015-01-17 20:29:20)

Include a space before single quote and bang when pretty-printing
This fix includes a refactoring. I would need to pass yet another boolean (setPreferSingleQuotes) on the options, which in the original design means yet another overload of the parsePrint method. It's a smell that all the callsites of this method are in other utility classes (not in test* methods) and they pass false, true arguments which don't mean anything unless you find that positional argument to parsePrint to see what that boolean controls.
So instead I've made a minimal interface so that each callsite can explicitly set the options it cares about. It's more lines overall, but more readable IMHO. This also makes it easy to let individual tests alter the options for one particular assertion, as I needed to do.

I'd be happy to put the refactoring in a separate PR if you want.


Patch:
diff --git a/src/com/google/javascript/jscomp/CodeConsumer.java b/src/com/google/javascript/jscomp/CodeConsumer.java
index 772ed482e..08d892e1b 100644
--- a/src/com/google/javascript/jscomp/CodeConsumer.java
+++ b/src/com/google/javascript/jscomp/CodeConsumer.java
@@ -199,7 +199,7 @@ abstract class CodeConsumer {
       // is valid and should print like
       // / // / /
       append(" ");
-    } else if (c == '"' && isWordChar(getLastChar())) {
+    } else if ((c == '"' || c == '\'') && isWordChar(getLastChar())) {
       maybeInsertSpace();
     }
 
diff --git a/src/com/google/javascript/jscomp/CodePrinter.java b/src/com/google/javascript/jscomp/CodePrinter.java
index ce37c46d7..6031d134a 100644
--- a/src/com/google/javascript/jscomp/CodePrinter.java
+++ b/src/com/google/javascript/jscomp/CodePrinter.java
@@ -333,14 +333,12 @@ public final class CodePrinter {
 
     @Override
     void appendOp(String op, boolean binOp) {
+      if (getLastChar() != ' ' && op.charAt(0) != ',') {
+        append(" ");
+      }
+      append(op);
       if (binOp) {
-        if (getLastChar() != ' ' && op.charAt(0) != ',') {
-          append(" ");
-        }
-        append(op);
         append(" ");
-      } else {
-        append(op);
       }
     }
 


Test patch:
diff --git a/test/com/google/javascript/jscomp/CodePrinterTest.java b/test/com/google/javascript/jscomp/CodePrinterTest.java
index 58d92987a..d50662157 100644
--- a/test/com/google/javascript/jscomp/CodePrinterTest.java
+++ b/test/com/google/javascript/jscomp/CodePrinterTest.java
@@ -104,58 +104,19 @@ public class CodePrinterTest extends TestCase {
     return new CodePrinter.Builder(parse(js)).setCompilerOptions(options).build();
   }
 
-  CompilerOptions newCompilerOptions(boolean prettyprint, int lineThreshold) {
+  private abstract class CompilerOptionBuilder {
+    abstract void setOptions(CompilerOptions options);
+  }
+
+  CompilerOptions newCompilerOptions(CompilerOptionBuilder builder) {
     CompilerOptions options = new CompilerOptions();
     options.setTrustedStrings(trustedStrings);
     options.preserveTypeAnnotations = preserveTypeAnnotations;
     options.setLanguageOut(languageMode);
-    options.setPrettyPrint(prettyprint);
-    options.setLineLengthThreshold(lineThreshold);
+    builder.setOptions(options);
     return options;
   }
 
-  CompilerOptions newCompilerOptions(boolean prettyprint, int lineThreshold, boolean lineBreak) {
-    CompilerOptions options = newCompilerOptions(prettyprint, lineThreshold);
-    options.setLineBreak(lineBreak);
-    return options;
-  }
-
-  String parsePrint(String js, boolean prettyprint, int lineThreshold) {
-    return parsePrint(js, newCompilerOptions(prettyprint, lineThreshold));
-  }
-
-  String parsePrint(String js, boolean prettyprint, boolean lineBreak, int lineThreshold) {
-    return parsePrint(js, newCompilerOptions(prettyprint, lineThreshold, lineBreak));
-  }
-
-  String parsePrint(String js, boolean prettyprint, boolean lineBreak,
-      boolean preferLineBreakAtEof, int lineThreshold) {
-    CompilerOptions options = newCompilerOptions(prettyprint, lineThreshold, lineBreak);
-    options.setPreferLineBreakAtEndOfFile(preferLineBreakAtEof);
-    return parsePrint(js, options);
-  }
-
-  String parsePrint(String js, boolean prettyprint, boolean lineBreak, int lineThreshold,
-      boolean outputTypes) {
-    return new CodePrinter.Builder(parse(js, true))
-        .setCompilerOptions(newCompilerOptions(prettyprint, lineThreshold, lineBreak))
-        .setOutputTypes(outputTypes)
-        .setTypeRegistry(lastCompiler.getTypeRegistry())
-        .build();
-  }
-
-  String parsePrint(String js, boolean prettyprint, boolean lineBreak,
-                    int lineThreshold, boolean outputTypes,
-                    boolean tagAsStrict) {
-    return new CodePrinter.Builder(parse(js, true))
-        .setCompilerOptions(newCompilerOptions(prettyprint, lineThreshold, lineBreak))
-        .setOutputTypes(outputTypes)
-        .setTypeRegistry(lastCompiler.getTypeRegistry())
-        .setTagAsStrict(tagAsStrict)
-        .build();
-  }
-
-
   String printNode(Node n) {
     CompilerOptions options = new CompilerOptions();
     options.setLineLengthThreshold(CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD);
@@ -641,7 +602,12 @@ public class CodePrinterTest extends TestCase {
   private void assertPrint(String js, String expected) {
     parse(expected); // validate the expected string is valid JS
     assertEquals(expected,
-        parsePrint(js, false, CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineLengthThreshold(CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD);
+          }
+        })));
   }
 
   private void assertPrintSame(String js) {
@@ -717,8 +683,14 @@ public class CodePrinterTest extends TestCase {
 
   private void assertLineBreak(String js, String expected) {
     assertEquals(expected,
-        parsePrint(js, false, true,
-            CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineBreak(true);
+            options.setLineLengthThreshold(CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD);
+          }
+        })));
   }
 
   public void testPreferLineBreakAtEndOfFile() {
@@ -754,9 +726,23 @@ public class CodePrinterTest extends TestCase {
   private void assertLineBreakAtEndOfFile(String js,
       String expectedWithoutBreakAtEnd, String expectedWithBreakAtEnd) {
     assertEquals(expectedWithoutBreakAtEnd,
-        parsePrint(js, false, false, false, 30));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineBreak(false);
+            options.setLineLengthThreshold(30);
+            options.setPreferLineBreakAtEndOfFile(false);
+          }
+        })));
     assertEquals(expectedWithBreakAtEnd,
-        parsePrint(js, false, false, true, 30));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineBreak(false);
+            options.setLineLengthThreshold(30);
+            options.setPreferLineBreakAtEndOfFile(true);
+          }
+        })));
   }
 
   public void testPrettyPrinter() {
@@ -967,6 +953,28 @@ public class CodePrinterTest extends TestCase {
         "}\n");
   }
 
+  // For https://github.com/google/closure-compiler/issues/782
+  public void testPrettyPrinter_spaceBeforeSingleQuote() throws Exception {
+    assertPrettyPrint("var f = function() { return 'hello';};",
+        "var f = function() {\n" +
+            "  return 'hello';\n" +
+            "};\n",
+        new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPreferSingleQuotes(true);
+          }
+        });
+  }
+
+  // For https://github.com/google/closure-compiler/issues/782
+  public void testPrettyPrinter_spaceBeforeBangOperator() throws Exception {
+    assertPrettyPrint("var f = function() { return !b; };",
+        "var f = function() {\n" +
+        "  return !b;\n" +
+        "};\n");
+  }
+
   public void testTypeAnnotations() {
     assertTypeAnnotations(
         "/** @constructor */ function Foo(){}",
@@ -1169,15 +1177,39 @@ public class CodePrinterTest extends TestCase {
   }
 
   private void assertPrettyPrint(String js, String expected) {
+    assertPrettyPrint(js, expected, new CompilerOptionBuilder() {
+      @Override void setOptions(CompilerOptions options) { }
+    });
+  }
+
+  private void assertPrettyPrint(String js, String expected,
+                                 final CompilerOptionBuilder optionBuilder) {
     assertEquals(expected,
-        parsePrint(js, true, false,
-            CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(true);
+            options.setLineBreak(false);
+            options.setLineLengthThreshold(CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD);
+            optionBuilder.setOptions(options);
+          }
+        })));
   }
 
   private void assertTypeAnnotations(String js, String expected) {
     assertEquals(expected,
-        parsePrint(js, true, false,
-            CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD, true));
+        new CodePrinter.Builder(parse(js, true))
+            .setCompilerOptions(newCompilerOptions(new CompilerOptionBuilder() {
+              @Override
+              void setOptions(CompilerOptions options) {
+                options.setPrettyPrint(true);
+                options.setLineBreak(false);
+                options.setLineLengthThreshold(CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD);
+              }
+            }))
+            .setOutputTypes(true)
+            .setTypeRegistry(lastCompiler.getTypeRegistry())
+            .build());
   }
 
   public void testSubtraction() {
@@ -1246,7 +1278,14 @@ public class CodePrinterTest extends TestCase {
 
   private void assertLineLength(String js, String expected) {
     assertEquals(expected,
-        parsePrint(js, false, true, 10));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineBreak(true);
+            options.setLineLengthThreshold(10);
+          }
+        })));
   }
 
   public void testParsePrintParse() {
@@ -1600,7 +1639,19 @@ public class CodePrinterTest extends TestCase {
   }
 
   public void testStrict() {
-    String result = parsePrint("var x", false, false, 0, false, true);
+    String result = new CodePrinter.Builder(parse("var x", true))
+        .setCompilerOptions(newCompilerOptions(new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineBreak(false);
+            options.setLineLengthThreshold(0);
+          }
+        }))
+        .setOutputTypes(false)
+        .setTypeRegistry(lastCompiler.getTypeRegistry())
+        .setTagAsStrict(true)
+        .build();
     assertEquals("'use strict';var x", result);
   }