migrate to fs2-io - #257
Conversation
|
Btw, is there any reason not to use formatter plugin? |
|
dep #258 |
|
Wow, nice, thanks for working on this! I'm happy to help review this one :) |
| @@ -78,6 +82,12 @@ object TextInput { | |||
| TextInput[F](path, docType, InputRuntime.textFileResource[F](file, codec).map(StreamReader(_, file.length.toInt)), Some(file)) | |||
| def fromStream[F[_]: Sync] (path: Path, docType: TextDocumentType, stream: F[InputStream], codec: Codec, autoClose: Boolean): TextInput[F] = | |||
There was a problem hiding this comment.
Btw, I think all uses of F[InputStream] should actually be replaced with fs2.Stream[F, Byte]. It is a referentially transparent, pure-FP, and fs2.io includes methods to get fs2.Stream from classpath resources etc. while correctly handling blocking.
There was a problem hiding this comment.
Yes, I feel the same. However, I think we should be careful because i/o stream is such a fundamental component for io module that it will propagate changes.
There was a problem hiding this comment.
Unfortunately I have not yet looked at this PR in detail, but as a general tendency, I feel like while it would make sense to standardise on fs2.Stream for any internal API wherever feasible, I think I'd still like to offer InputStream in the public APIs, since it is such a common type and this lib is also intended for an audience that does not know that much about Typelevel libs. The implementation of such a method could then immediately translate down to fs2 wherever possible.
There was a problem hiding this comment.
I think I'd still like to offer
InputStreamin the public APIs, since it is such a common type and this lib is also intended for an audience that does not know that much about Typelevel libs.
💯 I definitely agree with this sentiment, with regard to accessibility.
However, in this case I would argue that signatures F[InputStream] are actually more likely to be abused by the uninformed user than fs2.Stream[F, Byte].
A user who doesn't know much about Cats Effect could very plausibly write F.pure where they meant F.delay or even worse F.blocking and cause all sorts of problems for Laika/Cats Effect which make many important assumptions about the semantics of code suspended in F[_] (for example, at which point side-effects vs pure are evaluated and on which threadpool).
On the other hand, It seems reasonable to add scaladocs to point a user to methods such as fs2.io.readClassResource("myResource") to get an fs2.Stream[F, Byte], which I think are easy to use, hard to abuse, and do the right thing out-of-the-box.
To offer an accessible "unsafe" API, FWIW I think you should do away with F[_] entirely which I suspect is confusing for this group of users as well. This API can be then implemented in an imperative style and take the appropriate measures not to violate the assumptions of the Cats Effect runtime.
There was a problem hiding this comment.
I was actually thinking about the specific use case of interop with existing tools that provide an InputStream, not the simpler scenarios of classpath or file resources. Removing the existing fromStream method does not make anything safer here, since it just moves the problem to fs2.io.readInputStream which also expects an F[InputStream]. The way I see it it's just more code to write for the user and the same kind of trap to fall into.
My preference would be not to remove any existing entry-point API, but instead only add fromStream(fs2.io.Stream) and then use fs2 for most of the internal API. Which means to also keep fromFile(String) and fromClasspathResource(String), etc.
There was a problem hiding this comment.
But it's a good point to improve scaladoc for users not familiar with cats-effect. From working with junior devs in my day job I am aware that confusing F.delay, F.pure and F.blocking is amongst the no1 sources of bugs when using effects (to some degree even for seniors, to my surprise).
|
Thank you @armanbilge for taking time to review this PR. I will tweak this pr in a few days. |
e1420eb to
946f9d0
Compare
|
Apologies for the very late and incomplete feedback you received on this PR! The main problem was that I was unable to spot some of the issues here just by browsing the PR without actually working on this locally myself, which I did not find the time for until last week. I'll be closing this as superseded by #281 now which is an umbrella ticket that lists all the PRs for the fs2 migration. There are several issues that would have needed to be addressed in this PR and discussing them all in a PR that attempts to do the whole migration in one step appeared too difficult and time-consuming. Some, but not all, of the issues were:
Thanks a lot for kicking this off and investing some time into this! I promise I try to give feedback earlier in the future or at least recommend to suspend the work until I performed some basic sanity checks on the chosen approach. |
|
Closing as superseded by #281 |
|
Thank you for feedback to this PR and proposing better gradual migration PRs! |
3 participants
Hello.
In this PR, I replaced java.io stuffs with fs2.io.Files and fs2.io.Path.
I tried to hide changes and keep user-facing signature as possible except for
Sync/Asyncconstraint, but I think some API changes are unavoidable.It makes sense to use fs2 library for io module despite additional dependencies because fs2 helps cross platform io operations as mentioned in #231. It will be great if laika-io runs on js too.