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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
