Skip to content

Convert lambda block to expression after adopting assertThrows#582

Merged
timtebeek merged 6 commits intoopenrewrite:mainfrom
shivanisky:main
Aug 16, 2024
Merged

Convert lambda block to expression after adopting assertThrows#582
timtebeek merged 6 commits intoopenrewrite:mainfrom
shivanisky:main

Conversation

@shivanisky
Copy link
Copy Markdown
Collaborator

@shivanisky shivanisky commented Aug 15, 2024

Test case for inlining valid single line assertthrows

What's changed?

Inline valid single line assert throws test case

What's your motivation?

This recipe creates intellij suggestion/warning to inline this code and I much prefer it inlined, saves 2 lines of useless code.

Anyone you would like to review specifically?

@timtebeek

Have you considered any alternatives or workarounds?

Yes - maybe a separate recipe which achieves this is better, or maybe both - change this recipe, and have a separate recipe. Can do separate recipe first as it addresses at a global level.

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

Test case for inlining valid single line assertthrows
@timtebeek timtebeek changed the title Update UpdateTestAnnotationTest.java Inline single line assertThrows with method invocation Aug 15, 2024
@timtebeek timtebeek marked this pull request as draft August 15, 2024 15:58
@timtebeek
Copy link
Copy Markdown
Member

Thanks for yet another case to cover; think indeed best here to have a separate recipe, as that then keeps the logic cleaner here, and allows us to pick up cases already converted.

@shivanisky
Copy link
Copy Markdown
Collaborator Author

shivanisky commented Aug 15, 2024 via email

@timtebeek
Copy link
Copy Markdown
Member

I think we already have this one; we just need to tie it in here. Nice to be able to leverage existing logic there! :)

https://github.com/openrewrite/rewrite-static-analysis/blob/f1e53d203de026938615fee8b5175cba379dc279/src/test/java/org/openrewrite/staticanalysis/LambdaBlockToExpressionTest.java#L35-L58

@timtebeek
Copy link
Copy Markdown
Member

Did a quick check: we're running up against this safe guard where any overload invalidates the visit of a lambda argument.
https://github.com/openrewrite/rewrite-static-analysis/blob/6d04813574c6b02c8bf7d6e2f66b2bdaa881854c/src/main/java/org/openrewrite/staticanalysis/LambdaBlockToExpression.java#L74

Not quite sure if we can safely change that here. 🤔

@timtebeek
Copy link
Copy Markdown
Member

Specifically, the methods were seeing overloaded at that particular point are these.

org.junit.jupiter.api.Assertions{
    name=assertThrows,
    return=Generic{T extends java.lang.Throwable},
    parameters=[java.lang.Class<Generic{T extends java.lang.Throwable}>,org.junit.jupiter.api.function.Executable,java.util.function.Supplier<java.lang.String>]
}
org.junit.jupiter.api.Assertions{
    name=assertThrows,
    return=Generic{T extends java.lang.Throwable},
    parameters=[java.lang.Class<Generic{T extends java.lang.Throwable}>,org.junit.jupiter.api.function.Executable,java.lang.String]
}
org.junit.jupiter.api.Assertions{
    name=assertThrows,
    return=Generic{T extends java.lang.Throwable},
    parameters=[java.lang.Class<Generic{T extends java.lang.Throwable}>,org.junit.jupiter.api.function.Executable]
}

We could try this out in LambdaBlockToExpressionTest to see if we can safely recognize these cases and allow the replacement, but it'll be tricky not the break any other cases there. I'll leave it at this for now. :)

@timtebeek

This comment was marked as outdated.

@timtebeek timtebeek marked this pull request as ready for review August 16, 2024 10:06
@timtebeek timtebeek self-requested a review August 16, 2024 10:06
@timtebeek timtebeek changed the title Inline single line assertThrows with method invocation Convert lambda block to expression after adopting assertThrows Aug 16, 2024
@timtebeek timtebeek added the enhancement New feature or request label Aug 16, 2024
@timtebeek timtebeek merged commit da544f8 into openrewrite:main Aug 16, 2024
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

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants