[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:1970
+  but this behavior is changed by another option,
+  ``JavaStaticImportAfterImport``.
 
----------------
MyDeveloperDay wrote:
> Can you add a test that shows if the sorting is still in the groups, i.e. I can't tell if
> 
> ```
> import com.test.a;
> import static org.a;
> 
> import com.test.b;
> import static org.b;
> ```
> 
> becomes
> 
> ```
> import static org.a;
> import static org.b;
> import com.test.a;
> 
> import com.test.b;
> ```
> 
> or
> 
> ```
> import static org.a;
> import static org.b;
> 
> import com.test.a;
> 
> import com.test.b;
> ```
> 
> or
> 
> ```
> import static org.a;
> import com.test.a;
> 
> import static org.b;
> import com.test.b;
> 
> ```
> 
> 
> 
> 
> 
Done.


================
Comment at: clang/include/clang/Format/Format.h:1708
+  /// \endcode
+  bool JavaStaticImportAfterImport;
+
----------------
JakeMerdichAMD wrote:
> MyDeveloperDay wrote:
> > Can we consider changing the name or the option to make it clearer what its for?
> > 
> > `SortJavaStaticImport`
> > 
> > and make it an enumeration rather than true/false
> > 
> > `Before,After,Never`
> > 
> > There need to be a "don't touch it option (.e.g. Never)" that does what it does today (and that should be the default, otherwise we end up causing clang-format to change ALL java code" which could be 100's of millions of lines+
> > 
> > 
> I'm confused, there is not currently a never option... Right now the behavior is always 'before', there is no 'after' or 'never'.
> 
> Making it an enum would probably be more ergonomic though, even there is no never option.
If `SortIncludes` is `true`, it doesn't make sense to not touch Java static include.
Making it an enum is also good for me.



================
Comment at: clang/unittests/Format/SortImportsTestJava.cpp:253
 
+TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) {
+  FmtStyle.JavaStaticImportAfterImport = true;
----------------
JakeMerdichAMD wrote:
> MyDeveloperDay wrote:
> > please test all options
> > 
> > You are also missing a test for checking the PARSING of the options
> Parsing test was already noted and done (unless this changes to an enum of course). But testing the 'false' case here makes sense.
Done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201