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
Kotlin 1.4 dependency upgrade and language features #5947
Conversation
792191c
to
b847069
Compare
I don't think it helps us? I can't think of a place where explicit |
okhttp-logging-interceptor/src/test/java/okhttp3/logging/HttpLoggingInterceptorTest.java
Outdated
Show resolved
Hide resolved
@swankjesse explicit API mode isn't about code clarity as much as library authors ensuring they're intentional about public APIs, since it's easy to accidentally do that in kotlin's public-by-default model |
Another thing you can do here is remove the explicit stdlib dependencies in build.gradle files |
@ZacSweers Thanks for the thorough review, I think after this lands a silent code cleanup might be a good next step. Pick up Kotlin IDE suggestions as well. |
@swankjesse we ready? |
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.
Yay! The fun interfaces are nice.
id 'ru.vyarus.animalsniffer' version '1.5.0' | ||
id 'com.github.johnrengelman.shadow' version '5.2.0' | ||
id 'me.champeau.gradle.japicmp' version '0.2.9' | ||
id "ru.vyarus.animalsniffer" version "1.5.0" |
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.
The ' quotes don't do interpolation, the " quotes do. Motivation here?
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 that isn't how my brain works, I wouldn't use different strings based on whether it needs interpolation. I'd just mainly use the interpolated form whether or not is has a variable part.
okhttp-testing-support/src/main/kotlin/okhttp3/RecordingEventListener.kt
Outdated
Show resolved
Hide resolved
@@ -378,18 +375,18 @@ fun Socket.peerName(): String { | |||
* @param source the source used to read bytes from the socket. | |||
*/ | |||
fun Socket.isHealthy(source: BufferedSource): Boolean { | |||
try { | |||
return try { |
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.
For what it's worth, the return value here is non-obvious with the finally clause. IntelliJ is obnoxious by making recommendations that make the code less clear.
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.
The specific reason I like the return here is that it forces a return expression from all branches of the clause. Compiler would fail here anyway, but makes it clear that the result of the try catch is the final result of the method.
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.
Can I argue this one?
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.
Of course. And your argument makes sense.
Update to Kotlin 1.4 and basic upgrade to functionality. This also includes okio 2.8.0.