Skip to content

Add WrapExpensiveLogStatementsInConditionals to Slf4jBestPractices #216

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Apr 30, 2025
Merged

Add WrapExpensiveLogStatementsInConditionals to Slf4jBestPractices #216

merged 9 commits into from
Apr 30, 2025

Conversation

timtebeek
Copy link
Member

What's your motivation?

Not having to run this recipe individually, or having to remember to add it to a composite

Anyone you would like to review specifically?

This now fails org.openrewrite.java.logging.slf4j.Slf4jBestPracticesTest#exceptionIsAppendedAtEndOfLogMessage; I wonder if we should even add these conditional blocks for log levels warn & error, as they are hardly ever disabled. Maybe make it optionally configurable and only add blocks up to info by default?

diff --git a/Test.java b/Test.java
index 2ee8c4f..fab82d5 100644
--- a/Test.java
+++ b/Test.java
@@ -6,9 +6,11 @@ 
         try {
           throw new IllegalStateException("oops");
         } catch (Exception e) {
-          logger.error("aaa: {}", e);
-          logger.error("bbb: {}", String.valueOf(e));
-          logger.error("ccc: {}", e.toString());
+            if (logger.isErrorEnabled()) {
+                logger.error("aaa: {}", e);
+                logger.error("bbb: {}", String.valueOf(e));
+                logger.error("ccc: {}", e.toString());
+            }
         }
     }
 }
\ No newline at end of file

Any additional context

@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Apr 30, 2025
github-actions[bot]
github-actions bot reviewed Apr 30, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 29-30
    • lines 38-39
    • lines 54-56
    • lines 64-66
    • lines 727-726
    • lines 735-734
    • lines 781-800

github-actions[bot]
github-actions bot reviewed Apr 30, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 29-30
    • lines 38-39
    • lines 54-56
    • lines 64-66
    • lines 760-759
    • lines 768-767
    • lines 814-833

github-actions[bot]
github-actions bot reviewed Apr 30, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 29-30
    • lines 38-39
    • lines 54-56
    • lines 64-66
    • lines 768-767
    • lines 814-833

github-actions[bot]
github-actions bot reviewed Apr 30, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 29-30
    • lines 38-39
    • lines 54-56
    • lines 64-66
    • lines 768-767
    • lines 814-833

@timtebeek timtebeek self-assigned this Apr 30, 2025
@timtebeek timtebeek marked this pull request as ready for review April 30, 2025 11:31
@timtebeek timtebeek moved this from In Progress to Ready to Review in OpenRewrite Apr 30, 2025
JohannisK
JohannisK reviewed Apr 30, 2025
View reviewed changes
src/main/java/org/openrewrite/java/logging/slf4j/InexpensiveSLF4JLoggers.java Outdated
@@ -66,78 +52,78 @@ public String getDisplayName() {
@Override
public String getDescription() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the description reflect the restriction in loglevels it operates on?

github-actions[bot]
github-actions bot reviewed Apr 30, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 29-30
    • lines 38-39
    • lines 54-56
    • lines 64-66
    • lines 768-767
    • lines 814-833

github-actions[bot]
github-actions bot reviewed Apr 30, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 29-30
    • lines 38-39
    • lines 54-56
    • lines 64-66
    • lines 768-767
    • lines 814-833

@timtebeek timtebeek changed the title Add InexpensiveSLF4JLoggers to Slf4jBestPractices Add WrapExpensiveLogStatementsInConditionals to Slf4jBestPractices Apr 30, 2025
github-actions[bot]
github-actions bot reviewed Apr 30, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/resources/META-INF/rewrite/examples.yml
    • lines 29-30
    • lines 38-39
    • lines 54-56
    • lines 64-66
    • lines 814-833
    • lines 961-960

@timtebeek timtebeek merged commit d1e0c69 into main Apr 30, 2025
1 of 2 checks passed
@timtebeek timtebeek deleted the add-InexpensiveSLF4JLoggers-to-Slf4jBestPractices branch April 30, 2025 11:53
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@JohannisK JohannisK JohannisK approved these changes

@greg-at-moderne greg-at-moderne Awaiting requested review from greg-at-moderne

Assignees

@timtebeek timtebeek

Labels
None yet
Projects
OpenRewrite
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timtebeek @JohannisK