-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Update Kotlin to version 2.1.0 #44809
Update Kotlin to version 2.1.0 #44809
Conversation
Thanks for your pull request! Your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
b458ffd
to
55f2f23
Compare
Thanks for taking care of this! |
Can you please rebase onto main? |
c578c94
to
6404e78
Compare
Thanks for the reminder. I forgot I didn't push my last commit and undraft this. It should be updated and hopefully the CI passes now. |
Thanks! |
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview 0ac0e12 has been successfully built and deployed to https://quarkus-pr-main-44809-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
CI didn't really run on this which is really weird |
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.
Thanks for this effort, greatly appreciated.
I have a question though and I would like to get it sorted out before we merge. See inline.
@@ -12,6 +12,6 @@ class CountryNamePayloadTransformer { | |||
@Outgoing("countries-t1-out") | |||
suspend fun transform(country: Country): Country { | |||
delay(100) | |||
return Country(country.name.toUpperCase(), country.capital) | |||
return Country(country.name.lowercase(), country.capital) |
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.
Is it expected that you changed toUpperCase()
to lowercase()
? It might make sense but it looks odd to me?
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.
Yes, this is actually required. The toLowerCase
method has been deprecated with a warning for a while but now they removed it or changed it to an error (I don't remember exactly any more, but the project didn't compile any longer). So this is a needed fix and lowercase()
is the intended replacement.
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 would expect uppercase()
to be the replacement for toUpperCase()
:).
6404e78
to
f629bf6
Compare
This comment has been minimized.
This comment has been minimized.
Seems like
cc @jmartisk |
Right, I'll try to upgrade |
Thanks! |
Thanks! Sorry I wasn't able too look for the errors in time. I'm unfortunately a bit swamped right now. |
This comment has been minimized.
This comment has been minimized.
Waiting for smallrye/smallrye-graphql#2234 to land in Quarkus. |
f629bf6
to
9e64b27
Compare
@andreas-eberle probably a good idea to include these two changes in this PR:
A new version of SmallRye GraphQL was released so hopefully we will be able to make progress on this one soon! |
9e64b27
to
d36a7f3
Compare
@gsmet: I've quickly updated the coroutines library and also the smallrye graphql dependency. Let's see if that already fixes the issues. |
Yeah so the GraphQL one won't work as the update breaks things. We will have to wait for the SmallRye GraphQL team to push a proper update. |
Thanks for the info @gsmet. Then I'll rebase the PR as soon as the graphql update is properly integrated. |
@andreas-eberle I've submitted #45228 that also incorporates your commit (and the coroutines upgrades mentioned earlier), I think we need to apply both upgrades at the same time. Please check it |
Thanks @jmartisk. Looks good for me from the kotlin update side. Let's see what CI says about your PR. I will close this one. |
Closing in favor of #45228 |
This PR updates Kotlin to the newly released version 2.1.0