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

MyDeveloperDay added a comment. @stasm thank you for taking it on, "Commandeer" the revision via the Revision actions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116638/new/ https://reviews.llvm.org/D116638 ___ cfe-commits mailing list

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

stasm added a comment. In D116638#3547366 , @andmis wrote: > In D116638#3545246 , @stasm wrote: > >> I'm still interested in seeing this fixed. Would it help if I rebased this >> change and addressed the

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

andmis added a comment. In D116638#3545246 , @stasm wrote: > I'm still interested in seeing this fixed. Would it help if I rebased this > change and addressed the outstanding review comments? Go for it! I don't plan to do any further work on this. (I'm

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

stasm added a comment. Herald added a project: All. I'm still interested in seeing this fixed. Would it help if I rebased this change and addressed the outstanding review comments? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116638/new/ https://reviews.llvm.org/D116638

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

MyDeveloperDay added a comment. please mark your review comments as done when done so we know its a complete review CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116638/new/ https://reviews.llvm.org/D116638 ___ cfe-commits mailing list

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

stasm added a comment. The reporter of issue 52935 here. Thanks, @andmis, for your work. Thinking about the `ColumnLimit: 0` and `JavaScriptWrapImports: false` case, it seems that there are two issues in the current implementation that could

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

andmis added a comment. The current behavior when `ColumnLimit: 0` and `JavaScriptWrapImports: false` formats this: import {aaa} from "abc"; import {aaa, bbb, ccc} from "def"; import {aaa, bbb} from "defghi"; import {aaa, long, ccc,} from "ghi"; import { aaa,

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

MyDeveloperDay added a comment. > applies just as well to the proposal of force-wrapping at >= 2 imports Absolutely.. but it justifies that this option has got to the point that its no longer one thing or the other based on our personal subjective opinions... now we need to break down what we

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

andmis added a comment. My guess that `ColumnLimit: 0` is rarely used for JS is based on the objective fact that JS import formatting is (IMO very) buggy with the column limit set that way, and it took several years for us to hear a bug report about it. And "we should not make assumptions

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

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

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

MyDeveloperDay added a comment. > (1) very few people use... I don't think it's actually the behavior people > will want Subjective or Objective opinion? https://github.com/search?o=desc=%22ColumnLimit%3A+0%22=indexed=Code 95,000+ occurrences of "ColumnLimit" in github YAML files and

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

andmis added a comment. Thanks for the feedback. Two things: 1. Force-breaking at >= 2 imports and not breaking at 1 import feels has the advantage of being simple to state, implement, document, and test, but I don't think it's actually the behavior people will want. For example, the original

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

MyDeveloperDay added a comment. I would say for the `ColumnLimit:0` case, we don't have to wrap a single import like this: for `JavaScriptWrapImports :true` import { Get } from '@nestjs/common'; For more than one import then I'd say it should do: import { Get, Req }

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

That's what happens when you hit the column limit, when there is a column limit. But do we really want every one-symbol import to wrap to 3 lines when `ColumnLimit: 0`? Slash to force the user to unwrap every import, even 20-symbol 300-column imports, to a single line? On Wed, Jan 5, 2022 at

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

MyDeveloperDay added a comment. > Make JavaScriptWrapImports: true *always* wrap imports to multiple lines. > This will be noisy and ugly. Isn't this what `prettier` does when effectively the ColumnLimit is exceeded. i.e. import { Controller, Get, Post, Req } from '@nestjs/common'; becomes

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

andmis 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

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

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

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

andmis added a comment. Thanks all for the reviews. I've updated the patch and added responses to your comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:2820 +**JavaScriptWrapImports** (``Boolean``) :versionbadge:`clang-format 3.9` + Whether to wrap JavaScript

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

andmis updated this revision to Diff 397579. andmis added a comment. - Move source code for option documentation to `Format.h`, from which the RST is autogenerated. - Clean up tests in response to review feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116638/new/

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

HazardyKnusperkeks 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

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

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

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

MyDeveloperDay added inline comments. Comment at: clang/unittests/Format/FormatTestJS.cpp:1948 + Style.JavaScriptWrapImports = false; verifyFormat("import {VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying," " VeryLongImportsAreAnnoying,

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

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