Skip to content

Recipe to convert parametrized JUL calls to slf4j #160

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

Merged
merged 9 commits into from
Jul 1, 2024

Conversation

woj-tek
Copy link
Contributor

@woj-tek woj-tek commented Jun 27, 2024

What's changed?

Recipe to convert parametrized JUL Logger calls to slf4j and enabled relevant unit-test

What's your motivation?

Continuing work on #155

Anything in particular you'd like reviewers to focus on?

Initial draft PR as suggested by Tim

Anyone you would like to review specifically?

@timtebeek

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@woj-tek woj-tek marked this pull request as draft June 27, 2024 17:49
…rizedArguments.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek timtebeek self-requested a review June 28, 2024 08:24
@timtebeek timtebeek added the recipe Recipe Requested label Jun 28, 2024
@timtebeek
Copy link
Member

Great work again @woj-tek ; I've already pushed up some polishing commits, mostly to help guide the implementation. You'll notice that the tests still fail due to a lingering Log.Level import; that's because there's likely a left over type reference on the method returned. I suspect this will be because we do not change the argument types.

I've also added another test to indicate that we should not change the effectively logged argument order.

Both issues are easiest to resolve if we switch to using JavaTemplate, with an appropriate classpath entry for SLF4J, to compose the new method to return. That should set the right type on the generated method, and gives you flexibility in reordering the arguments.

@timtebeek timtebeek marked this pull request as ready for review July 1, 2024 19:50
Copy link
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started @woj-tek ! There were some fun little edge cases, but I think we mostly got it working now. Let me know how this works for you based on our snapshot versions

@timtebeek timtebeek merged commit 25726e9 into openrewrite:main Jul 1, 2024
2 checks passed
@woj-tek
Copy link
Contributor Author

woj-tek commented Jul 8, 2024

Thanks for getting this started @woj-tek ! There were some fun little edge cases, but I think we mostly got it working now.

Sorry, I was carried away with other tasks ans couldn't take a look at this - and kinda got stuck with JavaTemplates.

Btw. do you think it would be possible to create simplified API for template akin to slf4j parametrised calls with template being method(#{}, #{}), etc and then .addParameter() taking appropriate declaration instead? (though with LSTs probably not possible). Another thing that was somewhat problematic was missing javadocs in various places which would help a lot with understanding things (and differences between them) without the need to jumping between guides :-)

Let me know how this works for you based on our snapshot versions

I checked it and it seems I missed one call with Exception (Throwable) eg. log(Level level, String msg, Throwable thrown) and others:

} catch (Exception e) {
    log.log(Level.WARNING, "Something went wrong", e);

I'll update/add test cases for those as well.

@timtebeek timtebeek deleted the parametrized-jul-to-slf4j branch July 8, 2024 14:15
@timtebeek
Copy link
Member

As to the JavaTemplate proposal above, we do already support #{any()} which I feel is pretty close to what you're asking for, right? The .addParameter(...) is somewhat unlikely given how we've structured and cache JavaTemplate currently. One can build up an reuse a JavaTemplate and the classpath it needs, provided the parameters are only passing when actually applied. Hope that's understandable there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe Requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants