-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add recipe to migrate ResponseStatusException
changes
#598
Conversation
@Laurens-W any input from your side here seeing that you recently added this similar Anything we can reuse from there for instance? |
.../openrewrite/java/spring/framework/MigrateResponseStatusExceptionGetRawStatusCodeMethod.java
Show resolved
Hide resolved
.../openrewrite/java/spring/framework/MigrateResponseStatusExceptionGetRawStatusCodeMethod.java
Show resolved
Hide resolved
.../openrewrite/java/spring/framework/MigrateResponseStatusExceptionGetRawStatusCodeMethod.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/spring/framework/MigrateResponseStatusExceptionTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is about as perfect of a pull request as I have ever seen; thanks a lot @mslowiak ! Just a small final question/suggestion before we merge, which we can also pick up on our end if needed.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
import org.springframework.web.server.ResponseStatusException; | ||
class A { | ||
void foo(ResponseStatusException e) { | ||
HttpStatus i = e.getStatusCode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this ought to be HttpStatusCode
instead of HttpStatus
, as per:
https://docs.spring.io/spring-framework/docs/6.0.0/javadoc-api/org/springframework/web/ErrorResponseException.html#getStatusCode()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Laurens-W is that something you can work in based on your earlier work on #589 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging this iteration already as it's an improvement already; we'd need to change the test in a follow up PR to expect the variable and import change, and create a new dedicated recipe to handle the ResponseStatusException getStatus()
.
.../openrewrite/java/spring/framework/MigrateResponseStatusExceptionGetRawStatusCodeMethod.java
Outdated
Show resolved
Hide resolved
displayName: Migrate breaking changes in `ResponseStatusException` | ||
description: Migrate Spring Framework 5.3's `ResponseStatusException` method `getRawStatusCode()` to Spring Framework 6's `getStatusCode().value()` and `ResponseStatusException` method `getStatus()` to Spring Framework 6's `getStatusCode()` . | ||
recipeList: | ||
- org.openrewrite.java.spring.framework.MigrateResponseStatusExceptionGetRawStatusCodeMethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps once the recipe for getStatus
to getStatusCode
is in place these could be included below line 33, or the recipe on line 33 could be pulled into here and this sub recipe could be transformed into a HttpStatus
to HttpStatusCode
recipe
What's changed?
Add recipe to migrate breaking changes introduced in Spring Framework 6.0 touching
ResponseStatusException
class.What's your motivation?
Anything in particular you'd like reviewers to focus on?
Was not sure where to put the recipes - I added them to
spring-framework-60.yml
as another group (? - let me know correct naming here)Checklist