Added format and optimize imports entry point. #92

sormuras · 2016-11-02T09:37:21Z

This PR introduces a new entry point in Formatter, that format an input string (a Java compilation unit) and optimize imports into an output string.

The new entry point formatSource(String, JavadocOnlyImports ) behaves similar to the Main/FormatFileCallable combo, that allows optimizing imports in Java source files. It provides a better manner for external application to invoke the expected behaviour of google-java-format programmatically -- without utilizing the Main/FormatFileCallable combo path discussed here diffplug/spotless#48

sormuras · 2016-11-07T18:54:33Z

@cushon Any chance to merge this or a similar entry point?

cushon · 2016-11-10T16:57:16Z

core/src/main/java/com/google/googlejavaformat/java/Formatter.java

+   * @param joi if not {@code null}, the imports are optimized accordingly
+   * @return the output string
+   * @throws FormatterException if the input string cannot be parsed
+   */


JavadocOnlyImports is an experiment, let's leave it out of the API for now.

If overloading formatSource(String) does not work out, what about formatSourceAndOptimizeImports(String)?

cushon · 2016-11-10T16:58:01Z

core/src/main/java/com/google/googlejavaformat/java/Formatter.java

+   * @return the output string
+   * @throws FormatterException if the input string cannot be parsed
+   */
+  public String formatSource(String input, JavadocOnlyImports joi) throws FormatterException {


I think you can use formatSource(String) instead?

Tried that first, but there were some test failing - will list them here.

One of the now failing tests, is an assertion test, that imports are not touched by default:

@Test public void importsNotReorderedByDefault()

org.junit.ComparisonFailure: bad output for B21647014 <Click to see difference> org.junit.ComparisonFailure: bad output for B21647014 <Click to see difference> org.junit.ComparisonFailure: bad output for B21647014 <Click to see difference> org.junit.ComparisonFailure: bad output for B21647014 <Click to see difference> org.junit.ComparisonFailure: bad output for M <Click to see difference> org.junit.ComparisonFailure: bad output for M <Click to see difference> org.junit.ComparisonFailure: bad output for M <Click to see difference> org.junit.ComparisonFailure: bad output for M <Click to see difference> org.junit.ComparisonFailure: bad output for Unformatted <Click to see difference> org.junit.ComparisonFailure: bad output for Unformatted <Click to see difference> org.junit.ComparisonFailure: bad output for Unformatted <Click to see difference> org.junit.ComparisonFailure: bad output for Unformatted <Click to see difference> org.junit.ComparisonFailure: <Click to see difference> at com.google.common.truth.Platform.comparisonFailure(Platform.java:133) at com.google.common.truth.Truth$1.failComparing(Truth.java:77) at com.google.common.truth.StringSubject.isEqualTo(StringSubject.java:69) at com.google.googlejavaformat.java.FormatterTest.importsNotReorderedByDefault(FormatterTest.java:249) org.junit.ComparisonFailure: <Click to see difference> at com.google.googlejavaformat.java.JavadocFormattingTest.doFormatTest(JavadocFormattingTest.java:1269) at com.google.googlejavaformat.java.JavadocFormattingTest.brAtSignBug(JavadocFormattingTest.java:823)

The initial blocks of four failures are from FormatterIntegrationTest.idempotent... parameterized test.

I vote for keeping the new method name for the new behaviour.

cushon · 2016-11-10T16:58:50Z

core/src/main/java/com/google/googlejavaformat/java/Formatter.java

+    String output = formatSource(input, Collections.singleton(Range.closedOpen(0, input.length())));
+    if (joi != null) {
+      output = RemoveUnusedImports.removeUnusedImports(output, joi);
+      output = ImportOrderer.reorderImports(output);


What other artifacts? Is there a bug for that? If there are issues with the import pass, let's fix that instead of formatting everything twice.

Multiple empty lines between package and class declaration are left behind by the import pass.

Instead of

package com.google.example; public class ExampleTest {}

it produces

package com.google.example; public class ExampleTest {}

What about keeping the extra pass with a TODO marker, open a new ticket for it - and solving it afterwards?

Found the reason: in private static JCCompilationUnit parse(Context context, String javaInput) the javaInput String provided by the test uses \n as line separator - but the JCCompilationUnit uses \r\n, presumably the system default line separator.

~~Do you know off-hand, how pass our guessed line separator to the context, parser, thing from com.sun.source.WhatEver?~~

Wrong.

Ordering of reorder imports and remove unused imports matters. The empty lines left over are those empty lines, that already existed in the original (re-ordered) input.

So, a second pass is necessary. The end location could be less than output.length, i.e. point to the line contains the first type declaration.

Or do it like:

reorder imports (at most 2 blocks of import statements)

remove unused imports (at most 2 empty lines above the first type declaration)

format

sormuras · 2016-11-10T20:42:43Z

@cushon I refactored the entry point to Formatter.formatSourceAndOptimizeImports(String), leaving JavadocOnlyImports out of the API.

Concerning the method name: if we re-use the already published formatSource(String) some test fail. See details in the outdated comments above.

I also added an ignored test to MainTest:

  @Test
  @Ignore("FormatFileCallable.call() needs a second format run after fixing imports")
  public void optimizeImportsDoesNotLeaveEmptyLines()
  ...

that demonstrates the artifacts when format is executed before fixing imports. I tried to re-order the FormatFileCallable.call() method as:

@Override
public String call() throws FormatterException {
  String fixed = fixImports(input);
  if (parameters.fixImportsOnly()) {
    return fixed;
  }

  Formatter formatter = new Formatter(options);
  return formatter.formatSource(fixed, characterRanges(fixed).asRanges());
}

but that leads to many other failing assertions, mainly error message checking ... new issue?

sormuras · 2016-11-15T23:02:30Z

Anything left to do, @cushon?

sormuras · 2017-01-18T13:24:41Z

@cushon Still interested in merging this? If yes, I'll resolve the conflicts in Formatter.java

jbduncan

Hi @sormuras, I found a couple of minor things in this PR which I think deserve your attention. :)

jbduncan · 2017-04-10T00:19:27Z

core/src/main/java/com/google/googlejavaformat/java/Formatter.java

 import com.google.googlejavaformat.Newlines;
 import com.google.googlejavaformat.Op;
 import com.google.googlejavaformat.OpsBuilder;
+import com.google.googlejavaformat.java.RemoveUnusedImports.JavadocOnlyImports;


@sormuras Is this import needed any longer?

jbduncan · 2017-04-10T00:22:29Z

core/src/test/java/com/google/googlejavaformat/java/FormatterTest.java


+  @Test
+  public void importsOptimizedIfRequested() throws FormatterException {
+    String input =


@sormuras Does not Google Java Style dictate that indents in multiline statements must be +4, or am I missing something here?

cgrushko · 2017-10-11T21:47:00Z

Any update on this?
I was surprised to find out that the formatter behaves differently on the command-line and in the IntelliJ plugin.

cushon · 2017-10-11T21:56:55Z

+@plumpy

I was surprised to find out that the formatter behaves differently on the command-line and in the IntelliJ plugin.

The IntelliJ plugin leaves import sorting and unused import removal to IntelliJ. The import fixing operates on the entire file instead of only handling changes lines, and when we did the Eclipse integration it really didn't want the formatter touch anything outside of the specific ranges it requested. Maybe IntelliJ handles that better?

Anyway, IntelliJ's built-in import removal is going to be better integrated and more accurate, since it has a compiler front-end instead of identifying uses of imports syntactically. And it's easy to set up IntelliJ to use the new import order. So I'm not sure we actually want to use GJF's import handling there?

sormuras · 2017-10-11T22:00:39Z

I'd still be happy to resolve the conflicts. :)

cgrushko · 2017-10-11T22:02:58Z

@cushon it's an extra thing to configure (which I didn't know about).
But sure :)
Is there some guide on how to do this? perhaps put it in README.md?

cushon · 2017-10-12T03:09:55Z

If changing the configuration is the best option, documenting that in the README sounds good. But I was hoping @plumpy would have a better idea about how to make it automatic :)

plumpy · 2017-10-12T03:50:05Z

@cushon I can look into it. There is some additional configuration which we do automatically for Google-internal installs of IntelliJ, so that's why I had never really noticed gjf wasn't the one changing the imports. @cgrushko was using a non-Google install at the time, so that's why he did notice :)

I'll see if I can force IntelliJ to use gjf's import style when it's enabled.

opporancisis · 2018-03-29T20:48:26Z

Dear supporters of this project. Is there any way to fix the issue with imports ordering in Intellij IDEA?

Current behavior:

install plugin, enable it
open any class that previously was in a bad format
Ctrl+Alt+L
= code is formatted as expected
Ctrl+Alt+O
= imports are order in a different order comparing to Google rules - IDEA uses some default settings
./gradlew goJF
= ordering of imports is fixed in java code as expected

So, I'm too lazy to go to Code Style and fix ordering manually. And I don't want to ask my colleagues to do this as well.

jbduncan · 2018-03-29T21:34:46Z

Hi @opporancisis. I admit that I'm a little unclear on how your comment is related to this PR, but I'll do my best to answer your query. :)

I'm a bit confused as to why you're invoking Ctrl+Alt+O to sort imports. Does not the plugin sort imports as google-java-format's Formatter.formatSourceAndOptimizeImports entry point does?

If so, does not ./gradlew goJF solve the problem by auto-sorting the import? (I presume that you're using this plugin: https://github.com/sherter/google-java-format-gradle-plugin). Or is it too inconvenient for your and your colleagues' taste? :)

opporancisis · 2018-03-29T21:49:50Z

Hi @jbduncan. Sorry if I started to ask questions in a wrong thread.

why you're invoking Ctrl+Alt+O

In general it's very convenient and useful hot key. You remove unused imports and sort them.
Isn't it a common use case?

does not ./gradlew goJF solve the problem by auto-sorting the import?

It solves ordering, but I want Ctrl+Alt+O as well.
I can imaging that there may be such workflow:

user commits code with unordered imports.
build server makes additional commit during pull request merge - commit that sorts imports with the help of grade goJF. I.e. automatic fix of code style discrepancies.

Do people use such configuration in a real life?

jbduncan · 2018-03-29T23:35:48Z

Sorry if I started to ask questions in a wrong thread.

No worries! Sorry if I came across poorly.

I think as a general rule, it's best to start a new issue even if an existing closed issue/PR seems relevant because, the way GitHub works, people are more likely to miss conversations on closed issues/PRs.

In general it's very convenient and useful hot key. You remove unused imports and sort them.
Isn't it a common use case?

I agree! I'm personally used to running the relevant command using Gradle or IntelliJ's support for Gradle, though (as I'm a user/contributor of Spotless, an alternative plugin which also supports google-java-format).

But having read your comment above, it makes a lot of sense to me that many people prefer using their IDE's built-in command for this. :)

Do people use such configuration in a real life?

I'm sure people do, but by my understanding it's not recommended for various reasons, i.e. it's better to format the code manually with ./gradlew goJF before committing, and at CI time to check that all Java source files are formatted properly and fail otherwise.

I wrote about this before on one or two other issues/PRs somewhere on GitHub, but I can't seem to find them. I'll get back to you if I can find them again. :)

googlebot added the cla: yes label Nov 2, 2016

cushon reviewed Nov 10, 2016

View reviewed changes

Added format and optimize imports entry point.

f983738

This was referenced Nov 17, 2016

IntelliJ plugin should sort imports in group alphabetically. But the plugin do not sort. #94

Closed

Fix imports when formatting by default. #96

Closed

jbduncan reviewed Apr 10, 2017

View reviewed changes

cushon closed this in adf8837 Oct 13, 2017

sormuras deleted the optimize_imports branch October 13, 2017 06:34

Added format and optimize imports entry point. #92

Added format and optimize imports entry point. #92

Uh oh!

Conversation

sormuras commented Nov 2, 2016

Uh oh!

sormuras commented Nov 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sormuras Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sormuras Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sormuras commented Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sormuras commented Nov 15, 2016

Uh oh!

sormuras commented Jan 18, 2017

Uh oh!

jbduncan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgrushko commented Oct 11, 2017

Uh oh!

cushon commented Oct 11, 2017

Uh oh!

sormuras commented Oct 11, 2017

Uh oh!

cgrushko commented Oct 11, 2017

Uh oh!

cushon commented Oct 12, 2017

Uh oh!

plumpy commented Oct 12, 2017

Uh oh!

opporancisis commented Mar 29, 2018

Uh oh!

jbduncan commented Mar 29, 2018

Uh oh!

opporancisis commented Mar 29, 2018

Uh oh!

jbduncan commented Mar 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sormuras Nov 10, 2016 •

edited

Loading

sormuras Nov 10, 2016 •

edited

Loading

sormuras commented Nov 10, 2016 •

edited

Loading