Skip to content
This repository was archived by the owner on Mar 29, 2021. It is now read-only.

Propose an implicit val for the default instance for Console[IO] to the user#22

Merged
gvolpe merged 2 commits intoprofunktor:masterfrom
guizmaii:master
Jan 7, 2019
Merged

Propose an implicit val for the default instance for Console[IO] to the user#22
gvolpe merged 2 commits intoprofunktor:masterfrom
guizmaii:master

Conversation

@guizmaii
Copy link
Copy Markdown
Contributor

@guizmaii guizmaii commented Jan 5, 2019

Hi @gvolpe,

While working on the update of advanced-http4s, I remarked that you don't propose an implicit val for the default instance of Console[IO].

It's useful when you work with code like this:

  def stream[F[_]: Console]: F[Unit] = ...

  override def run(args: List[String]): IO[ExitCode] = {
    import cats.effect.Console.implicits._

    stream[IO].as(ExitCode.Success)
  }

WDYT ?

@gvolpe
Copy link
Copy Markdown
Member

gvolpe commented Jan 6, 2019

I can't remember exactly why we didn't make that val io: Console[IO] = SyncConsole.stdio[IO] implicit but I'm thinking it should be. So there would be no need to introduce some extra object. @kubukoz thoughts?

@kubukoz
Copy link
Copy Markdown
Member

kubukoz commented Jan 6, 2019

I thought Console isn't a typeclass and as it's and algebra it'd be better not to have an implicit instance (one can create an IO Console for tests and accidentally not use it). I like the proposed solution with the implicits object. That'll require an import to bring in the instance, and the above issue will be less common.

Comment thread core/src/main/scala/cats/effect/Console.scala Outdated
@gvolpe
Copy link
Copy Markdown
Member

gvolpe commented Jan 6, 2019

I think that was the reason 👍 . I like the idea of implicits.io._

@guizmaii
Copy link
Copy Markdown
Contributor Author

guizmaii commented Jan 7, 2019

@gvolpe the import will have this form: import cats.effect.Console.implicits._ or this one import cats.effect.Console.implicits.ioConsole but not implicits.io._

@gvolpe
Copy link
Copy Markdown
Member

gvolpe commented Jan 7, 2019

@guizmaii why not import cats.effect.Console.implicits.io? Having "console" twice seems kind of redundant.

@kubukoz
Copy link
Copy Markdown
Member

kubukoz commented Jan 7, 2019

That's just for the global namespace thing in think. Or does it not apply to implicit parameters (maybe just conversions)?

@guizmaii
Copy link
Copy Markdown
Contributor Author

guizmaii commented Jan 7, 2019

@gvolpe
As @kubukoz said:

ioConsole, which would be even better because implicits have kind of a global namespace and io will potentially clash very easily

It's safer.

@gvolpe
Copy link
Copy Markdown
Member

gvolpe commented Jan 7, 2019

Right on, sounds good to me! Will merge if no objections.

@gvolpe gvolpe merged commit 2d47863 into profunktor:master Jan 7, 2019
@guizmaii
Copy link
Copy Markdown
Contributor Author

guizmaii commented Jan 7, 2019

Thanks for your reviews @gvolpe @kubukoz :)

@gvolpe
Copy link
Copy Markdown
Member

gvolpe commented Jan 7, 2019

Thank you for putting up the work @guizmaii , I just updated the guide -> https://gvolpe.github.io/console4cats/guide.html

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants