openrewrite / rewrite-static-analysis

OpenRewrite recipes for identifying and fixing static analysis issues.
Apache License 2.0
31 stars 50 forks source link

Prevent unintentional changes in logging best practices that conflict with Google Java Format #354

Open koppor opened 1 week ago

koppor commented 1 week ago

What problem are you trying to solve?

I want to use Google Java Format (AOSP) together with OpenRewrite

Describe the solution you'd like

OpenRewrite should not write files if only whitespace change.

Have you considered any alternatives or workarounds?

Disabling rules.

Additional context

org.openrewrite.staticanalysis.ChainStringBuilderAppendCalls:

--- a/src/main/java/org/jabref/logic/exporter/GroupSerializer.java
+++ b/src/main/java/org/jabref/logic/exporter/GroupSerializer.java
@@ -29,8 +29,7 @@ public class GroupSerializer {
     private String serializeExplicitGroup(ExplicitGroup group) {
         StringBuilder sb = new StringBuilder();
         sb.append(MetadataSerializationConfiguration.EXPLICIT_GROUP_ID);
-        sb.append(
-                StringUtil.quote(
+        sb.append(StringUtil.quote(
                         group.getName(),
                         MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR,
                         MetadataSerializationConfiguration.GROUP_QUOTE_CHAR));

org.openrewrite.java.logging.slf4j.ParameterizedLogging

-                                    "dataModel JabRef52"
-                                            + " only supports pageInfo for the last citation of a group");
+                            LOGGER.warn("dataModel JabRef52" + " only supports pageInfo for the last citation of a group");

(For this, I can do a work-around by using single-line strings only)

Are you interested in contributing this feature to OpenRewrite?

timtebeek commented 1 week ago

hi @koppor ! I'm looking at the original classes but not seeing the same input there: https://github.com/JabRef/jabref/blob/ebca33e74ee674515e171036d11edc36e8ddc34e/src/main/java/org/jabref/logic/exporter/GroupSerializer.java#L29-L40

Are there additional changed out of view from the diff above that would explain why changes are reported in the first place? For both cases I suppose the best way to see these resolved is to replicate them in a minimal draft PR unit test, and then from there ensure the prefix is retained for the ChainStringBuilderAppendCalls case for instance.

koppor commented 1 week ago

The classes stem from https://github.com/koppor/jabref/pull/661.

I will try to create an MWE though.