Skip to content
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

Upgrade dependencies and fix for ktor 1.3.0. #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gchallen
Copy link

Fix dependency change to support ktor 1.3.0. Also upgrade kotlin and moshi dependencies, and fix test cases to support moshi 1.9.2.

@ArthurSav ArthurSav mentioned this pull request Feb 1, 2020
@@ -22,7 +24,9 @@ class MoshiConverter(private val moshi: Moshi = Moshi.Builder().build()) : Conte
val channel = request.value as? ByteReadChannel ?: return null
val source = channel.toInputStream().source().buffer()
val type = request.type
return moshi.adapter(type.javaObjectType).fromJson(source)
return withContext(Dispatchers.IO) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't we should change the coroutine context. It is already a suspending function, so it will pick up the given context when executed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bumped into this and did some investigation. Because fromJson() throws IOException, Intelij assumes it is doing a blocking call and warns that it is not safe to do in a coroutine. Using Dispatchers.IO silences this warning because it is "designed specifically to handle blocking calls".

I wasn't able to find any information on what ktor uses for ContentConverters. It is possible that it is already on using Dispatchers.IO, which mean calling it directly would be fine. But the IDE inspection doesn't have that context to know if it is a false positive.

I looked at the implementation of the of the ContentConverters for gson and serialization. Neither use IOException, so it is hard to tell if this is needed.

Copy link
Owner

@rharter rharter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue, otherwise looks great!

@@ -1,3 +1,5 @@
@file:Suppress("unused")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally warnings would be suppressed at their specific site, to be clear about what is being suppressed, why, and to avoid accidentally suppressing useful warning messages.

build.gradle.kts Outdated
id("com.github.ben-manes.versions") version "0.39.0"
}
subprojects {
group = "com.github.cs125-illinois"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This need to change back to the original values from gradle.properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants