[PATCH] D116638: [clang-format] Fix ignoring JavaScriptWrapImport when ColumnWidth: 0

[Martin Probst via Phabricator via cfe-commits]

mprobst added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2827
 
-**JavaScriptWrapImports** (``Boolean``) :versionbadge:`clang-format 3.9`
-  Whether to wrap JavaScript import/export statements.
+  * If ``ColumnWidth`` is 0 (no limit on the number of columns), then import
+    statements will keep the number of lines they start with.
----------------
this seems odd to me: my understanding is that clang-format always reflows the entire document, there's no logic to ever "keep" whitespace.

Are you sure you are seeing this behaviour? The logic change below sounds more as if the ColumnWidth: 0, import lines might not break (mustBreak returns false), but might still break?


================
Comment at: clang/unittests/Format/FormatTestJS.cpp:1975
+               Style);
+  // Using the input_code/expected version of verifyFormat because we can't
+  // indiscriminately test::messUp on these tests.
----------------
I'm not sure this test case really repros the doc changes you made (keeps any line wraps with ColW: 0).

I think if you wanted to demonstrate that, you'd need to add a test case where clang-format would normally make a change, and show that it does not with ColW: 0.

However I think that's not feasible: by design, clang-format cannot not make a change, it always reformats all code. You might need to rethink the intent here.


================
Comment at: clang/unittests/Format/FormatTestJS.cpp:1990
   Style.ColumnLimit = 40;
-  // Using this version of verifyFormat because test::messUp hides the issue.
+  Style.JavaScriptWrapImports = true;
   verifyFormat("import {\n"
----------------
curdeius wrote:
> It's already true, cf. line 1977. Remove this line.
FWIW, I think the tests might be more readable if each configuration ({true, false} x {col: 0, col: 40}) explicitly set all the options, even if it's a bit redundant.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116638/new/

https://reviews.llvm.org/D116638