Replace NetworkService with HttpClient in LCP#794
Conversation
| } | ||
| } | ||
| Try.success( | ||
| HttpFetchResponse(response.response, ByteArray(0)) |
There was a problem hiding this comment.
I don't understand why you return an empty ByteArray. You could return nothing. Besides, the error handling is a bit simplistic. That might be perfectionism, but in case of a filesystem error, we shouldn't fail with an HttpError. I would rather introduce an HttpDownloadError which could be either an HttpError or a FilesystemError.
There was a problem hiding this comment.
I added a HttpDownloadError sealed class in HttpError and followed the naming pattern in the latter: HttpDownloadError.Http and HttpDownloadError.Filesystem. I'm wary about naming conventions, as we already have an HttpError class. And there are a lot of files and classes already with the same name, which gets confusing and causes conflicts that have to be addressed.
Could also make HttpDownloadError an interface that HttpError implements, but that might be crossing boundaries a bit too much.
There was a problem hiding this comment.
And there are a lot of files and classes already with the same name, which gets confusing and causes conflicts that have to be addressed.
Could you elaborate on this?
There was a problem hiding this comment.
Ok! I think it's fine to have different types as they conform to different specifications. Even if they currently match, they might diverge at some point.
There was a problem hiding this comment.
Yeah having different types as fine, I'm more against the same naming.
2 participants
This PR replaces the usage of NetworkService with HttpClient in the LCP module.
Related to #779.