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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.


================
Comment at: clang/include/clang/Format/Format.h:2620
+  ///
+  /// * If ``ColumnWidth`` is 0 (no limit on the number of columns),
+  ///   then import statements will keep the number of lines they
----------------
Please change all occurrences of `ColumnWidth` (old name) to `ColumnLimit`.


================
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"
----------------
andmis wrote:
> mprobst wrote:
> > 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.
> I've reformatted them in two big blocks: one where `ColumnLimit > 0` and one where `ColumnLimit: 0`. So there are no more redundant option set statements.
> 
> This is the most-readable way to format the tests IMO because when `ColumnLimit > 0`, we can use the simple `verifyFormat` (where you just pass the expected result and it messes it up, formats, and checks) because clang-format will do a full reflow. But for the `ColumnLimit: 0` tests, we need to use the other `verifyFormat` (where we have input and expected output explicitly), and it's convenient to put a comment explaining what's happening at the top of the `ColumnLimit: 0` block.
It's ok for me too, but I'd like to either put all the involved options each time, or minimalistic changes, but not a mix of the two.


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

https://reviews.llvm.org/D116638