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

Provides effect type agnostic Console #256

Closed
wants to merge 3 commits into
base: master
from

Conversation

8 participants
@gvolpe
Contributor

gvolpe commented May 31, 2018

I think many of us were interested in at least having a putStrLn(str: String): IO[Unit] when IOApp was defined but it didn't happen and I missed that merge.

In this PR a Console[F[_]] datatype provides some common methods to write an read from the standard console. Don't know whether you guys think it's best to IOApp extends Console[IO] or pushing the decision to the user. I went for the first option but I'm open to suggestions.

Also, I used putStrLn, error and readLine as names matching the standard in Haskell except for readLine that it's getLine, I like it better :) Open to suggestions here as well.

/**
* Prints a message of type A to the standard console if an instance `Show[A]` is found.
*/
def putStrLn[A: Show](value: A)(implicit F: Sync[F]): F[Unit] = F.delay(println(value.show))

This comment has been minimized.

@alexandru

alexandru May 31, 2018

Member

This is pretty cool, but not future proof.

The Sync restriction is not enough to describe these functions via non-blocking I/O and it would be a pity if we don't allow for the possibility.

In fact on top of JavaScript println can only be implemented via console and in my experience its usage is pretty damaging, because it has an internal buffer which can overflow.

Having Sync as a restriction is a problem. Having Async as a restriction is a problem too.
Actually if this is an open trait, then the restrictions shouldn't be on the methods.

Timer doesn't add restrictions on the methods. And Timer isn't a supertype of IOApp either.
So how about modelling Console like Timer is?

And then IOApp can provide a def console: Console[IO] or something.

This comment has been minimized.

@gvolpe

gvolpe May 31, 2018

Contributor

It makes sense, don't know much about the JS internals but I agree that we should provide the option to do it asynchronously. Will follow the idea of Timer, thanks!

@gvolpe

This comment has been minimized.

Contributor

gvolpe commented May 31, 2018

@alexandru I wrote a default Console[IO] instance for jvm/IOAppPlatform but I'm not exactly sure what should be the instance for js/IOAppPlatform. Some guidance here would be much appreciated 😄

@@ -56,4 +57,12 @@ private[effect] object IOAppPlatform {
}

val defaultTimer: Timer[IO] = IOTimer.global

def defaultConsole: Console[IO] =

This comment has been minimized.

@oleg-py

oleg-py May 31, 2018

Contributor

I'm 👎 on having this on IOApp. IMO this kind of thing should be importable, and you shouldn't import anything from your Main

So I would rather have a syntax import, or something like that, for IO-specific version.

This comment has been minimized.

@gvolpe

gvolpe May 31, 2018

Contributor

The thing is where to define it? We can provide default instances for IO, for Sync[F] or for Async[F] for example and in most of the cases it would be convenient to have it together with IOApp, at least I find myself repeating this task all the time, but I'm open to suggestions.

What would you guys suggest?

@gvolpe

This comment has been minimized.

Contributor

gvolpe commented May 31, 2018

Okay so what about defining a default instance on Console.io? The method console will be removed from IOApp and the user could choose whether to use it or not by having import cats.effect.Console.io._.

We can also have a default Console.sync instance that works for any Sync[F] as a user opt. import cats.effect.Console.sync._.

@oleg-py

This comment has been minimized.

Contributor

oleg-py commented May 31, 2018

You can't have a sync instance - without making users ascribe the type all the time at least - but you can make a sync class (i.e. class SyncConsole[F[_]: Sync] extends Console[F] { ... }) and let users either pass it down (tagless style) or just roll their own (object taskConsole extends SyncConsole[Task]). Libraries based on cats-effect will be also free to provide their own instance of Console[F] 😏

@gvolpe

This comment has been minimized.

Contributor

gvolpe commented Jun 1, 2018

You're right @oleg-py , now that I'm looking at this with fresh eyes I like the SyncConsole[F] option as it is probably the least intrusive alternative and user opt-in. Will go for that one 👍

@gvolpe gvolpe force-pushed the gvolpe:master branch from 04c44df to 2d2900a Jun 1, 2018

/**
* Effect type agnostic `Console` with common methods to write and read from the standard console.
*/
@implicitNotFound("""Cannot find implicit value for Console[${F}].

This comment has been minimized.

@mpilquist

mpilquist Jun 1, 2018

Member

Do we really want to encourage implicit usage of Console? Seems like this doubles-down on the Timer design, where we are passing data types implicitly (not type classes).

This comment has been minimized.

@gvolpe

gvolpe Jun 1, 2018

Contributor

I'm fine with removing it 👍

/**
* Prints a message of type A to the standard console if an instance `Show[A]` is found.
*/
def putStrLn[A: Show](a: A): F[Unit]

This comment has been minimized.

@mpilquist

mpilquist Jun 1, 2018

Member

Add putStr too?

* For a given `F` data type fetches the implicit [[Console]]
* instance available implicitly in the local scope.
*/
def apply[F[_]](implicit console: Console[F]): Console[F] = console

This comment has been minimized.

@mpilquist

mpilquist Jun 1, 2018

Member

Same comment as above about encouraging implicit passing of a data type.

@implicitNotFound("""Cannot find implicit value for Console[${F}].
Note that ${F} needs to be at least a cats.effect.Sync data type.
""")
trait Console[F[_]] {

This comment has been minimized.

@mpilquist

mpilquist Jun 1, 2018

Member

Not a huge fan of the Haskell names here. Seems like print and println would be friendlier and align better with scala.Console.

This comment has been minimized.

@gvolpe

gvolpe Jun 1, 2018

Contributor

The only thing about having names such as print and println is that we might have conflicts with those defined by Scala. Consider this use case:

import cats.effect._

object console extends SyncConsole[IO]

import console._

val program: IO[Unit] =
  for {
    _ <- println("Enter your name: ") // Naming conflict
    _ <- readLine
    _ <- println(s"Hi $n!") // Naming conflict 
  }

This comment has been minimized.

@mpilquist

mpilquist Jun 1, 2018

Member

I find the create-object-that-extends-a-trait-and-import-all-members pattern pretty annoying so this doesn't really bother me. In this example, I'd much rather use IO.println(...) and IO.readLine. If writing polymorphic stuff, then perhaps like this:

def program[F[_]: Sync]: F[Unit] = {
  val console = SyncConsole.derive[F]
  for {
    _ <- console.println("Enter your name: ")
    _ <- console.readLine
    _ <- console.println(s"Hi $n!")
  } yield ()

I don't feel super strongly about this topic though. I'll happily use putStrLn if others are happy with it.

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Jun 1, 2018

Any thoughts regarding adding putStrLn as a method to IO as well?
I find it super useful when writing quick samples (when you don't bother being polymorphic), and almost never useful in real code, so a low ceremony IO.putStrLn("thing") would be nice

@gvolpe

This comment has been minimized.

Contributor

gvolpe commented Jun 1, 2018

I'm 👍 with that one @SystemFw , I keep on repeating myself all the time but I also do it in this way F.delay(println(value)) thus the addition of Console[F] 😃

@codecov-io

This comment has been minimized.

codecov-io commented Jun 1, 2018

Codecov Report

Merging #256 into master will decrease coverage by 0.5%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #256      +/-   ##
==========================================
- Coverage   88.97%   88.47%   -0.51%     
==========================================
  Files          63       64       +1     
  Lines        1624     1631       +7     
  Branches      172      178       +6     
==========================================
- Hits         1445     1443       -2     
- Misses        179      188       +9
@oleg-py

This comment has been minimized.

Contributor

oleg-py commented Jun 1, 2018

I feel really bad for having IO.putStrLn on the companion. It breaks cohesiveness for the sake of quick samples. Like if File class had a emailToAFriend method, really.


Would rather have an extra import (Console.io._) like proposed earlier

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Jun 1, 2018

@oleg-py I would agree if putStrLn was useful for anything but examples

@oleg-py

This comment has been minimized.

Contributor

oleg-py commented Jun 1, 2018

@SystemFw that's why I disagree — IMO the go-to effect type shouldn't be polluted with utilities only useful for short examples.

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Jun 1, 2018

@oleg-py I actually agree in general, it's just that println is so so common that it warrants an exception on one hand (unlike even print or readline), and easy enough to define that if one needs to go through a bit of ceremony to use it, might as well not exist at all.

Anyway, it's really not a hill I'm willing to die on, just that I've rewritten a convenient version of it so many times I'd really like to stop doing that :)

@oleg-py

This comment has been minimized.

Contributor

oleg-py commented Jun 1, 2018

it's just that println is so so common

In examples, perhaps. In real code, less so - often you work in F or Stream[F, ?] and the simplest way to add a debugging print will still be _ = println(...)


Assuming we have cats.effect.Console.io._, I think:

  1. Samples still have some imports required (e.g. cats._, implicits._, effect._), this just adds one extra which isn't that big of a deal.
  2. It's better to point readers of samples in the right direction straight away (e.g. if they would be looking for readLine - it's not on IO, but it's on console, and they would not have to figure out that it has to come from entirely different place).
  3. It's also better to not have two ways of doing exactly the same - unless we're going to add note to every snippet to encourage users to prefer Console.

Maybe I'm being unreasonable a bit, but having putStrLn on IO certainly makes me raise an eyebrow for these reasons 😅

@gvolpe

This comment has been minimized.

Contributor

gvolpe commented Jun 1, 2018

I'm okay with either solution, don't really mind having IO.putStrLn but I can do just fine with the extra import cats.effect.Console.io._. We need someone else to weigh on the decision though 😄 , I have the changes ready 😉

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Jun 2, 2018

the import is fine by me

@gvolpe gvolpe force-pushed the gvolpe:master branch from 2ef9cda to 6780563 Jun 2, 2018

Effect type agnostic Console with common operations to interact with …
…the standard console. Provides a defaul instance for any Sync[F].

@gvolpe gvolpe force-pushed the gvolpe:master branch from 6780563 to ab54c66 Jun 3, 2018

/**
* Prints a message to the standard console followed by a new line.
*/
def putStrLn(str: String): F[Unit] = putStrLn[String](str)

This comment has been minimized.

@notxcain

notxcain Jun 10, 2018

What do you think about marking this final, and two below as well?

This comment has been minimized.

@gvolpe

gvolpe Jun 11, 2018

Contributor

I have no strong feelings against it, can do it 👍

gvolpe added some commits Jun 11, 2018

@alexandru

Have been thinking about Console and the truth is I'd rather not have it in Cats-Effect.

The reason is that it's insufficiently well defined and we cannot do a better job in defining it without introducing complexity in Cats-Effect.

For example I was about to add a comment saying that I don't like def error as a name, because error is a noun and it works as a function when it's a builder / data constructor, like is the case for ExitCase.error.

But then I realized that the problem is actually larger than just naming. The problem is that users want to override the file handle they use for printing stuff.

This is why Java defines a PrintStream, and then System.out and System.err are just print streams pointing to the standard STDOUT and STDERR, so you have the same println function for both.

In other words, we don't actually want def error, what we want is a putStrLn that accepts a channel as an argument.

Similarly for readLine, Java defines System.in as an InputStream, meaning that the same logic you define for reading from the standard input can be applied to reading from files or network sockets.

Console is convenient, but it's not convenient enough and we'd be doing a worse job than Java's standard library at composability, introducing notions that are not reusable and only useful for "Hello, World!" type programs. Say what you will of Java's impurity and its blocking I/O, but the standard library achieves the sort of decoupling that we'd do well to learn from ;-)

And maybe it would be a useful endeavor to start building pure I/O abstractions for dealing with such basic tasks, but it cannot be part of Cats-Effect's core.

@gvolpe

This comment has been minimized.

Contributor

gvolpe commented Jun 11, 2018

Thanks for your thoughts @alexandru . I agree with you in some parts.

what we want is a putStrLn that accepts a channel as an argument.

This could be one way to do it. For example, fs2 defines Sink.showLinesStdOut and Sink.showLines defined as follow:

def showLines[F[_]: Sync, I: Show](out: PrintStream): Sink[F, I] = _.map(_.show).to(lines(out))  
def showLinesStdOut[F[_]: Sync, I: Show]: Sink[F, I] = showLines(Console.out)

A bunch of Haskell streaming libraries also define combinators to deal with the standard input / output in a streaming fashion and I find that very handy but of course they build on top of IO that already defines IO.putStrlLn and IO.readLine. See the streaming library for instance:

stdinLn :: MonadIO m => Stream (Of String) m () 
readLn :: (MonadIO m, Read a) => Stream (Of a) m () 
stdoutLn :: MonadIO m => Stream (Of String) m () -> m () 

Console is convenient, but it's not convenient enough and we'd be doing a worse job than Java's standard library at composability, introducing notions that are not reusable and only useful for "Hello, World!" type programs.

I disagree here. I think it's a nice to have "user-opt" datatype and nothing more.

And maybe it would be a useful endeavor to start building pure I/O abstractions for dealing with such basic tasks, but it cannot be part of Cats-Effect's core.

I don't know whether Console belongs in cats-effect core or not but what I'm sure about is that we need to provide it somewhere and right now there's no place else other than cats-effect.

@alexandru

This comment has been minimized.

Member

alexandru commented Jun 11, 2018

what we want is a putStrLn that accepts a channel as an argument.

This could be one way to do it. For example, fs2 defines Sink.showLinesStdOut and Sink.showLines defined as follow:

def showLines[F[]: Sync, I: Show](out: PrintStream): Sink[F, I] = .map(.show).to(lines(out))
def showLinesStdOut[F[
]: Sync, I: Show]: Sink[F, I] = showLines(Console.out)

That's not an alternative, as what you're showing piggybacks on Java's standard library. All streaming abstractions piggyback on top of operations that are oblivious of streaming.

At the end of the day, the most fine grained operation available to us is nothing more than AsynchronousFileChannel.write, which is basically:

def write(channel: Channel, content: Array[Byte], position: Int): IO[Unit]

See also Haskell's hPutStrLn.

@alexandru

This comment has been minimized.

Member

alexandru commented Jun 11, 2018

I disagree here. I think it's a nice to have "user-opt" datatype and nothing more.

Nice to have doesn't cut it imo.

When it comes to products and libraries, what you leave out is just as important as what you're including.

My objection to Console is that it answers a very specific use case that can be served by a Gist and doesn't build the user's understanding and capabilities. It doesn't scale, being something to avoid as soon as the user discovers something better. What we need is the equivalent for the aforementioned System.IO and that cannot be Cats-Effect, as I/O concerns are hard to get right.

@gvolpe

This comment has been minimized.

Contributor

gvolpe commented Jun 11, 2018

No objections, I get your points 👍

Maybe this should be part of a companion library to cats-effect? For example, in scalaz they are creating small projects that are easy to combine and compose together.

@alexandru

This comment has been minimized.

Member

alexandru commented Jun 11, 2018

Maybe this should be part of a companion library to cats-effect?

If we can come up with something sane, it can be an optional sub-project of cats-effect, but it's not an easy endeavor.

in scalaz they are creating small projects that are easy to combine and compose together.

I was under the impression that that's what we are doing, being Cats-Effect's entire raison d'être.

@alexandru

This comment has been minimized.

Member

alexandru commented Jun 11, 2018

I'm not sure how's best to proceed.

Awaiting input from others.

@rossabaker

This comment has been minimized.

Member

rossabaker commented Jun 11, 2018

In fs2, the IO utilities are relegated to the fs2-io project, which is highly useful. I can imagine a cats-effect-io, but I can also imagine a lot of sad gitter chats where we explain why type cats.effect.IO is not in module cats-effect-io.

@gvolpe

This comment has been minimized.

Contributor

gvolpe commented Jun 12, 2018

@rossabaker note that the methods I mentioned (Sink.showLinesStdOut) are part of fs2 core but maybe they shouldn't be there.

@gvolpe

This comment has been minimized.

Contributor

gvolpe commented Jun 25, 2018

@alexandru I'll close this until we get consensus or find a better solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment