-
Notifications
You must be signed in to change notification settings - Fork 605
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
Added debug and debugChunks combinators #1727
Conversation
@kubukoz How about this? |
* res0: List[Int] = List(1, 2, 3, 4) | ||
* }}} | ||
*/ | ||
def debug(tag: String = "", logger: String => Unit = println(_)): Stream[F, O] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe unsafeLogger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, the call site seems clunkier then -- s.debug(unsafeLogger = println)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking about side effects in the API. Arguably debug
itself suggests that there will be something shady going on (e.g. toString
for formatting), so maybe logger
will also be an acceptable trade-off :)
Looks very nice, I'll see what it looks like in some examples :) Although I'm starting to get worried about having so many methods (4 of them being for debugging). Maybe it's a good time to think about putting them in some importable object (as pipes), or an implicit class... |
Yeah, it's a tricky balance. I could see getting rid of the format varieties, leaving two methods. And moving them to somewhere else kind of defeats the point of having a quick / easily accessible way of debugging. |
Good point, it's probably a case of something you want to access quickly... but the API growing might eventually be a problem so we can think about it next time when adding new combinators :) Just checked the new functions, everything seemed fine - 👍 from me |
How about only leaving the |
@SystemFw it's more like |
Fixes #1569