Skip to content

Conversation

Riyazul555
Copy link
Contributor

@Riyazul555 Riyazul555 commented Jun 30, 2024

What's changed?

Explanation of Changes:
Argument Handling:

A check is added to detect if the Marker argument is present.
If a Marker is detected, it ensures that it remains as the first argument.
Argument Reordering:

After constructing the new argument list, if a Marker is detected, it is added at the beginning of the newArgList.
Test Adjustments:

Ensure that your tests verify that the Marker argument remains in the correct position and that the logging message is parameterized correctly.

What's your motivation?

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

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

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

@Riyazul555
Copy link
Contributor Author

@timtebeek please checkout this PR
Do tell me if any changes are required
Thanks

@timtebeek timtebeek self-requested a review June 30, 2024 09:29
@timtebeek timtebeek added the bug Something isn't working label Jun 30, 2024
@timtebeek timtebeek mentioned this pull request Jun 30, 2024
3 tasks
@Riyazul555
Copy link
Contributor Author

Any update on this PR @timtebeek ??

@timtebeek
Copy link
Member

I had a quick look already, but I'm out today. The class does not yet compile, and the test fails when I fix compilation. That'll need work still

@Riyazul555
Copy link
Contributor Author

Ok thanks @timtebeek
I guess there was some error with parenthesis of the test code
That's why compilation error I guess
Nevertheless

@timtebeek timtebeek marked this pull request as draft June 30, 2024 21:23
@timtebeek timtebeek marked this pull request as ready for review July 1, 2024 18:52
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 again for getting this started; I've pushed up the remaining changes to get it going.

@timtebeek timtebeek merged commit ec68564 into openrewrite:main Jul 1, 2024
@Riyazul555
Copy link
Contributor Author

Thanks @timtebeek

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

ParameterizedLogging recipe doesn't support Log4j2 Marker
3 participants