Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ImportLayoutStyle doesn't work as expected #2845

Open
tisonkun opened this issue Nov 11, 2022 · 20 comments
Open

ImportLayoutStyle doesn't work as expected #2845

tisonkun opened this issue Nov 11, 2022 · 20 comments
Labels
enhancement New feature or request

Comments

@tisonkun
Copy link

tisonkun commented Nov 11, 2022

            <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
Copy link
Author

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

@pway99
Copy link
Contributor

pway99 commented Nov 11, 2022

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
Copy link
Author

@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
Copy link
Contributor

pway99 commented Nov 15, 2022

Thanks @tisonkun, I will have a look

@traceyyoshima
Copy link
Contributor

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>

@traceyyoshima traceyyoshima added question Further information is requested and removed bug Something isn't working labels Nov 28, 2022
@tisonkun
Copy link
Author

@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
Copy link
Author

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
Copy link
Author

image

@traceyyoshima
Copy link
Contributor

traceyyoshima commented Nov 29, 2022

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
Copy link
Contributor

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.
  1. 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
Copy link
Author

tisonkun commented Nov 30, 2022

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
Copy link
Contributor

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.

  • Was it challenging to discover/find the documentation?
  • Were the details unclear about <activeStyles> and/or <activeRecipes>?

Thanks in advance!

@traceyyoshima
Copy link
Contributor

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
Copy link
Author

tisonkun commented Dec 6, 2022

@traceyyoshima

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

@tisonkun
Copy link
Author

tisonkun commented Dec 6, 2022

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
Copy link
Contributor

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 traceyyoshima transferred this issue from openrewrite/rewrite Dec 7, 2022
@traceyyoshima
Copy link
Contributor

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
Copy link
Contributor

Looping in @mike-solomon into this issue.

@traceyyoshima traceyyoshima added enhancement New feature or request and removed question Further information is requested labels Dec 8, 2022
@traceyyoshima traceyyoshima removed their assignment Dec 8, 2022
@traceyyoshima
Copy link
Contributor

@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
Copy link
Author

... 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) {
+}

@mike-solomon mike-solomon removed their assignment Feb 1, 2023
@sambsnyd sambsnyd transferred this issue from openrewrite/rewrite-docs Feb 14, 2023
@sambsnyd sambsnyd moved this to Backlog in OpenRewrite Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Backlog
Development

No branches or pull requests

5 participants