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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

Thanks for working on this!
Apart from other reviewer's comments, it looks pretty much ok, but the tests are a bit messy IMO.
I'd like to see something that tests 4 cases (false + no column limit, false + column limit, true + no column limit, true + column limit). In each case you should have the same imports formatted (possibly) differently.
It would be the best to have at least: single short import, single long import, multiple short import, multiple long import. And please keep what's currently tested.



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2836
 
-     true:
+     // Original
+     import {VeryLongImport, AnotherLongImport, LongImportsAreAnnoying} from 'some/module.js'
----------------
What does "Original" mean here? It's unclear whether the option is true or false nor what the ColumnWidth is.

I think you want to have 3 examples:
1) JavaScriptWrapImports: false, independent of ColumnWidth
2) JavaScriptWrapImports: true, ColumnWidth: 0
3) JavaScriptWrapImports: true, ColumnWidth: non-zero
In each case you want to have short and long import lines.


================
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"
----------------
It's already true, cf. line 1977. Remove this line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116638