[PATCH] D17385: clang-format: [JS] re-quote single or double quoted strings.

[Martin Probst via cfe-commits]

mprobst added inline comments.

================
Comment at: lib/Format/Format.cpp:1087
@@ +1086,3 @@
+        (Style.Quotes == FormatStyle::QS_Single &&
+         !FormatTok->TokenText.startswith("\"")) ||
+        (Style.Quotes == FormatStyle::QS_Double &&
----------------
djasper wrote:
> Are there more than the two options? Otherwise, I would find:
> 
>   Style.Quotes == FormatStyle::QS_Single && FormatTok->TokenText.startswith("\'")
> 
> more intuitive.
I would too, but think about `template strings`. Added a comment. I think it's safer to specifically check for the thing that we do handle/fix below, instead of just accepting everything we think is not correct.

================
Comment at: unittests/Format/FormatTestJS.cpp:638
@@ -637,3 +637,3 @@
 TEST_F(FormatTestJS, StringLiteralConcatenation) {
-  verifyFormat("var literal = 'hello ' +\n"
-               "    'world';");
+  verifyFormat("var literal =\n"
+               "    'hello ' + 'world';",
----------------
djasper wrote:
> Why change this test? I think this was meant to test that we always add a line break between two consecutive string literals as otherwise the user could just have written this without the "' + '".
This was exposed by the char_literal/string_literal test, it aligns the behaviour with what would always happen for a double quoted string. It fits the no-alignment rule.

I've changed the test to make sure it actually wraps on two lines.

================
Comment at: unittests/Format/FormatTestJS.cpp:878
@@ -876,3 +877,3 @@
                "  Y,\n"
-               "} from 'some/long/module.js';",
+               "} from\n    'some/long/module.js';",
                getGoogleJSStyleWithColumns(20));
----------------
djasper wrote:
> I don't think this is intended.
It is. This was incorrectly handled before, the line is longer than 20 chars. It's because I switched single quoted strings from char_literal to string_literal.


http://reviews.llvm.org/D17385