openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.09k stars 312 forks source link

ImportLayoutStyle doesn't work as expected #2845

Open tisonkun opened 1 year ago

tisonkun commented 1 year ago
            <plugin>
                <groupId>org.openrewrite.maven</groupId>
                <artifactId>rewrite-maven-plugin</artifactId>
                <version>${maven-rewrite-plugin.version}</version>
                <configuration>
                    <activeStyles>
                        <activeStyle>com.netflix.eureka.Style</activeStyle>
                    </activeStyles>
                    <activeRecipes>
                        <activeRecipe>org.openrewrite.java.format.AutoFormat</activeRecipe>
                    </activeRecipes>
                </configuration>
            </plugin>

doesn't expand star import after mvn rewrite:run:

import io.korandoru.zeronos.core.config.*;
import io.korandoru.zeronos.server.ZeronosServer;
import java.time.Instant;
import java.util.HashMap;
import lombok.Cleanup;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;

with manually formatted:

import io.korandoru.zeronos.core.config.ClusterConfig;
import io.korandoru.zeronos.core.config.ServerConfig;
import io.korandoru.zeronos.server.ZeronosServer;
import java.time.Instant;
import java.util.HashMap;
import lombok.Cleanup;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;
tisonkun commented 1 year ago

You may check it at https://github.com/tisonkun/zeronos/tree/openrewrite

pway99 commented 1 year ago

Hi @tisonkun, looking at your config org.openrewrite.java.format.AutoFormat does not order imports, can you try again with the org.openrewrite.java.OrderImports recipe added to the configuration?

tisonkun commented 1 year ago

@pway99 It seems that after adding this recipe, the import order changes, but the star import still be not expanded:

import io.korandoru.zeronos.core.config.*;
import io.korandoru.zeronos.server.ZeronosServer;
import lombok.Cleanup;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;

import java.time.Instant;
import java.util.HashMap;
pway99 commented 1 year ago

Thanks @tisonkun, I will have a look

traceyyoshima commented 1 year ago

Hi @tisonkun, thanks for reporting the issue!

I took a look at found there's been a slight miscommunication.

Context: "Organizing imports" is split into different recipes based on functionality. In this case, org.openrewrite.java.OrderImports will reorder and fold imports, and org.openrewrite.java.RemoveUnusedImports will remove unused imports and unfold when necessary.

Would you mind running the plugin with the following configuration?

            <plugin>
                <groupId>org.openrewrite.maven</groupId>
                <artifactId>rewrite-maven-plugin</artifactId>
                <version>${maven-rewrite-plugin.version}</version>
                <configuration>
                    <configLocation>tools/ci/rewrite.yml</configLocation>
                    <activeStyles>
                        <activeStyle>com.netflix.eureka.Style</activeStyle>
                    </activeStyles>
                    <activeRecipes>
                        <activeRecipe>org.openrewrite.java.format.AutoFormat</activeRecipe>
                        <activeRecipe>org.openrewrite.java.OrderImports</activeRecipe>
                        <activeRecipe>org.openrewrite.java.RemoveUnusedImports</activeRecipe>
                    </activeRecipes>
                </configuration>
            </plugin>
tisonkun commented 1 year ago

@traceyyoshima Thank you! I'll test it later.

BTW, I found that I cannot understand the different (usage) of recipes combining styles from the online docs. Is there any suggestion to understand how openrewrite works? (since I need to debug like in this issue)

tisonkun commented 1 year ago

Almost looks good. But one new issue here: the rewrite process cannot properly analyzes var variables.

That is, I define a variable in type RaftClientReply using final var reply = ...;, during the rewrite stage, it rewrites import org.apache.ratis.protocol.* to include import org.apache.ratis.protocol.RaftClientReply;, which can be removed.

tisonkun commented 1 year ago
image
traceyyoshima commented 1 year ago

Almost looks good. But one new issue here: the rewrite process cannot properly analyzes var variables.

That is, I define a variable in type RaftClientReply using final var reply = ...;, during the rewrite stage, it rewrites import org.apache.ratis.protocol.* to include import org.apache.ratis.protocol.RaftClientReply;, which can be removed.

@tisonkun I don't see any code like that in the sample project, so the results look fine. Are your latest changes pushed to the branch?

traceyyoshima commented 1 year ago

BTW, I found that I cannot understand the different (usage) of recipes combining styles from the online docs. Is there any suggestion to understand how openrewrite works? (since I need to debug like in this issue)

@tisonkun I'm not sure what you're referring to about combining style. There are two main styles:

  1. IntelliJ styles

    • IntelliJ styles are based on the IDE's default formatting and are a part of Autoformatting. Here is an example from java
    • Auto-formatting is based on languages like java, xml, and hcl, and each recipe applies basic formatting rules to the code.
    • Each language style may either be auto-detected (example of java) or manually configured either through a declarative recipe.
    • Auto-detected styles analyze the most common occurrences in the code base to preserve the existing formatting.
    • Manual configuration is found here in the docs. Note that the activeStyles you used here is a declared style based on the Netflix eurkea project.
  2. CheckStyle styles

    • I haven't worked much with CheckStyle, but the styles are based on CheckStyle configurations if a configuration is detected.

Does that answer your question or did you have something else in mind?

tisonkun commented 1 year ago

I'm not sure what you're referring to about combining style.

I mean the styles and recipes here:

                    <activeStyles>
                        <activeStyle>com.netflix.eureka.Style</activeStyle>
                    </activeStyles>
                    <activeRecipes>
                        <activeRecipe>org.openrewrite.java.format.AutoFormat</activeRecipe>
                        <activeRecipe>org.openrewrite.java.OrderImports</activeRecipe>
                        <activeRecipe>org.openrewrite.java.RemoveUnusedImports</activeRecipe>
                    </activeRecipes>
traceyyoshima commented 1 year ago

I mean the styles and recipes here:

Oh, okay - @tisonkun information on recipe configuration may be found here.

Would you mind sharing what you've found confusing about the docs? I.E.

Thanks in advance!

traceyyoshima commented 1 year ago

Hi @tisonkun, I'd like to check if you're issue was resolved. Do you need anything else or is it okay to close this issue?

tisonkun commented 1 year ago

@traceyyoshima

Sorry for the late response: I'm verifying it now and answer your questions above.

tisonkun commented 1 year ago

I don't see any code like that in the sample project

It's when you run the rewrite plugin locally, you will find an extra import occurs.

Were the details unclear about <activeStyles> and/or <activeRecipes>?

I can interpret recipes like steps/rules in spotless/checkstyle, but how can styles be mapped to? In other words, how the config (recipes and styles) loaded and applied during the plugin running.

traceyyoshima commented 1 year ago

It's when you run the rewrite plugin locally, you will find an extra import occurs.

Interesting -- okay, I'll republish everything locally and try this again in the afternoon. Thanks!

I can interpret recipes like steps/rules in spotless/checkstyle, but how can styles be mapped to? In other words, how the config (recipes and styles) loaded and applied during the plugin running.

@mike-solomon Can you take a look at this and provide your thoughts?

traceyyoshima commented 1 year ago

Hi @tisonkun, I've transferred the issue to rewrite-docs. We'll use this issue to track the work of adding more details about import layout styles, code styles, how recipes use the styles, and possibly other information.

Note: this isn't a high priority, so please be patient with us. :) Thanks in advance!

I'll check on the import issue, and create a new issue once it's reproduced.

tkvangorder commented 1 year ago

Looping in @mike-solomon into this issue.

traceyyoshima commented 1 year ago

@tisonkun, in regards to the additional import, I haven't been able to reproduce the issue locally: Screenshot 2022-12-08 at 11 20 33 AM

tisonkun commented 1 year ago

... Today I rerun the plugin with config:

            <plugin>
                <groupId>org.openrewrite.maven</groupId>
                <artifactId>rewrite-maven-plugin</artifactId>
                <version>${maven-rewrite-plugin.version}</version>
                <configuration>
                    <activeStyles>
                        <activeStyle>com.netflix.eureka.Style</activeStyle>
                    </activeStyles>
                    <activeRecipes>
                        <activeRecipe>org.openrewrite.java.format.AutoFormat</activeRecipe>
                    </activeRecipes>
                </configuration>
            </plugin>

But it generates strange diffs on indents and whitespace:

diff --git a/zeronos-client/src/test/java/io/korandoru/zeronos/client/ZeronosClientTest.java b/zeronos-client/src/test/java/io/korandoru/zeronos/client/ZeronosClientTest.java
index e6a6ef8..4d92e2b 100644
--- a/zeronos-client/src/test/java/io/korandoru/zeronos/client/ZeronosClientTest.java
+++ b/zeronos-client/src/test/java/io/korandoru/zeronos/client/ZeronosClientTest.java
@@ -31,9 +31,11 @@ public class ZeronosClientTest {
     public void testPutGet() throws Exception {
         final var serverConfig = ServerConfig.defaultConfig();
         final var clusterConfig = ClusterConfig.defaultConfig();
-        @Cleanup final var server = new ZeronosServer(serverConfig, clusterConfig, "n0");
+        @Cleanup
+        final var server = new ZeronosServer(serverConfig, clusterConfig, "n0");
         server.start();
-        @Cleanup final var client = new ZeronosClient(clusterConfig);
+        @Cleanup
+        final var client = new ZeronosClient(clusterConfig);

         final var dataMap = new HashMap<String, String>();
         for (int i = 0; i < 256; i++) {
diff --git a/zeronos-command/src/main/java/io/korandoru/zeronos/command/ZeronosCommandServerStart.java b/zeronos-command/src/main/java/io/korandoru/zeronos/command/ZeronosCommandServerStart.java
index 0fbeaf7..f44b6e9 100644
--- a/zeronos-command/src/main/java/io/korandoru/zeronos/command/ZeronosCommandServerStart.java
+++ b/zeronos-command/src/main/java/io/korandoru/zeronos/command/ZeronosCommandServerStart.java
@@ -78,7 +78,7 @@ public class ZeronosCommandServerStart implements Callable<Integer> {
                 ? ClusterConfig.readConfig(this.clusterConfig.toURI().toURL())
                 : ClusterConfig.defaultConfig();

-        final var server = new ZeronosServer(serverConfig, clusterConfig, serverOptions.id);
+        final var server = new ZeronosServer(serverConfig , clusterConfig , serverOptions.id);
         final var serverProcess = new ServerProcess(server);
         final var serverThread = new Thread(serverProcess);

diff --git a/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/ClusterConfig.java b/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/ClusterConfig.java
index 4622b63..e169b77 100644
--- a/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/ClusterConfig.java
+++ b/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/ClusterConfig.java
@@ -30,35 +30,36 @@ import java.util.UUID;

 public class ClusterConfig {

-    private final DocumentContext context;
+  private final DocumentContext context;

-    public static ClusterConfig defaultConfig() throws IOException {
-        return readConfig(ClusterConfig.class.getResource("/cluster.toml"));
-    }
+  public static ClusterConfig defaultConfig() throws IOException {
+    return readConfig(ClusterConfig.class.getResource("/cluster.toml"));
+  }

-    public static ClusterConfig readConfig(URL source) throws IOException {
-        final var mapper = new TomlMapper();
-        final var root = mapper.readTree(source);
-        final var config = Configuration.builder().mappingProvider(new JacksonMappingProvider()).build();
-        final var context = JsonPath.using(config).parse(root.toPrettyString());
-        return new ClusterConfig(context);
-    }
+  public static ClusterConfig readConfig(URL source) throws IOException {
+    final var mapper = new TomlMapper();
+    final var root = mapper.readTree(source);
+    final var config = Configuration.builder().mappingProvider(new JacksonMappingProvider()).build();
+    final var context = JsonPath.using(config).parse(root.toPrettyString());
+    return new ClusterConfig(context);
+  }

-    private ClusterConfig(DocumentContext context) {
-        this.context = context;
-    }
+  private ClusterConfig(DocumentContext context) {
+    this.context = context;
+  }

-    public UUID groupId() {
-        return UUID.fromString(context.read("$.group.id"));
-    }
+  public UUID groupId() {
+    return UUID.fromString(context.read("$.group.id"));
+  }

-    public List<RaftPeerModel> peers() {
-        return context.read("$.group.peer", new TypeRef<>() {});
-    }
+  public List<RaftPeerModel> peers() {
+    return context.read("$.group.peer", new TypeRef<>() {
+    });
+  }

-    @Override
-    public String toString() {
-        return context.jsonString();
-    }
+  @Override
+  public String toString() {
+    return context.jsonString();
+  }

 }
diff --git a/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/ServerConfig.java b/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/ServerConfig.java
index 82dbae0..463b830 100644
--- a/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/ServerConfig.java
+++ b/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/ServerConfig.java
@@ -27,43 +27,43 @@ import java.util.Optional;

 public class ServerConfig {

-    private final DocumentContext context;
+  private final DocumentContext context;

-    public static ServerConfig defaultConfig() throws IOException {
-        return readConfig(ServerConfig.class.getResource("/server.toml"));
-    }
+  public static ServerConfig defaultConfig() throws IOException {
+    return readConfig(ServerConfig.class.getResource("/server.toml"));
+  }

-    public static ServerConfig readConfig(URL source) throws IOException {
-        final var mapper = new TomlMapper();
-        final var root = mapper.readTree(source);
-        final var config = Configuration.builder().mappingProvider(new JacksonMappingProvider()).build();
-        final var context = JsonPath.using(config).parse(root.toPrettyString());
-        return new ServerConfig(context);
-    }
+  public static ServerConfig readConfig(URL source) throws IOException {
+    final var mapper = new TomlMapper();
+    final var root = mapper.readTree(source);
+    final var config = Configuration.builder().mappingProvider(new JacksonMappingProvider()).build();
+    final var context = JsonPath.using(config).parse(root.toPrettyString());
+    return new ServerConfig(context);
+  }

-    private ServerConfig(DocumentContext context) {
-        this.context = context;
-    }
+  private ServerConfig(DocumentContext context) {
+    this.context = context;
+  }

-    public String storageBasedir() {
-        return context.read("$.storage.basedir");
-    }
+  public String storageBasedir() {
+    return context.read("$.storage.basedir");
+  }

-    public boolean snapshotAutoTriggerEnabled() {
-        return findSnapshotAutoTriggerThreshold().isPresent();
-    }
+  public boolean snapshotAutoTriggerEnabled() {
+    return findSnapshotAutoTriggerThreshold().isPresent();
+  }

-    public long snapshotAutoTriggerThreshold() {
-        return findSnapshotAutoTriggerThreshold().orElse(400000L);
-    }
+  public long snapshotAutoTriggerThreshold() {
+    return findSnapshotAutoTriggerThreshold().orElse(400000L);
+  }

-    private Optional<Long> findSnapshotAutoTriggerThreshold() {
-        return Optional.ofNullable(context.read("$.snapshot.auto-trigger-threshold", Long.class));
-    }
+  private Optional<Long> findSnapshotAutoTriggerThreshold() {
+    return Optional.ofNullable(context.read("$.snapshot.auto-trigger-threshold", Long.class));
+  }

-    @Override
-    public String toString() {
-        return context.jsonString();
-    }
+  @Override
+  public String toString() {
+    return context.jsonString();
+  }

 }
diff --git a/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/model/RaftPeerModel.java b/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/model/RaftPeerModel.java
index 18cd015..642cf1a 100644
--- a/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/model/RaftPeerModel.java
+++ b/zeronos-core/src/main/java/io/korandoru/zeronos/core/config/model/RaftPeerModel.java
@@ -16,4 +16,5 @@

 package io.korandoru.zeronos.core.config.model;

-public record RaftPeerModel (String id, String address) {}
+public record RaftPeerModel (String id, String address) {
+}