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

feat(tail) Iterant.dump #439

Merged
merged 6 commits into from Sep 27, 2017

Conversation

Projects
None yet
2 participants
@Avasil
Collaborator

Avasil commented Sep 24, 2017

PR for #434

This is pretty straightforward, I've used map for printing because there is no foreach. On second thought I should probably write

case NextCursor(cursor, rest, stop) =>
          cursor.map { el =>
              out.println(s"$pos: $prefix --> $el")
              pos += 1
          }
          NextCursor[F, A](cursor, rest.map(loop), stop)

instead of

case NextCursor(cursor, rest, stop) =>
          val dumped = cursor.map { el =>
              out.println(s"$pos: $prefix --> $el")
              pos += 1
              el
          }
          NextCursor[F, A](dumped, rest.map(loop), stop)

but I will wait for your opinion and fix with any other comments

@codecov

This comment has been minimized.

codecov bot commented Sep 24, 2017

Codecov Report

Merging #439 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #439      +/-   ##
==========================================
+ Coverage   89.11%   89.17%   +0.05%     
==========================================
  Files         345      346       +1     
  Lines        9480     9502      +22     
  Branches     1232     1249      +17     
==========================================
+ Hits         8448     8473      +25     
+ Misses       1032     1029       -3
*/
def apply[F[_], A](source: Iterant[F, A], prefix: String, out: PrintStream = System.out)
(implicit F: Sync[F]): Iterant[F, A] = {
var pos = 0

This comment has been minimized.

@alexandru

alexandru Sep 24, 2017

Member

I don't like this var, for one because it breaks referential transparency even when the source is a Suspend.

Think of referential transparency as the ability to execute the thing multiple times and get the same result. By closing over this var you'll get different results on different runs, as you have shared mutable state.

You don't really need a var with your loop, when you can simply do:

def loop(pos: Long)(source: Iterant[F, A]): Iterant[F, A]
@alexandru

This comment has been minimized.

Member

alexandru commented Sep 24, 2017

@Avasil thanks for sending another PR 😀

On your question, it doesn't work, because a Cursor can have either strict or lazy behavior in map, depending on the underlying implementation ... so you can have cursors over arrays, which are strict, but you can have cursors over Iterator, which are lazy.

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Sep 24, 2017

@alexandru
You're right about var, actually this was my first approach but I've had some weird behaviour for batches/cursor but it turns out I was printing wrong index... facepalm

I still have some vars, but completely local, so it should be referentially transparent now but i can also write it like this, I'm not sure what's better:

val newPos = cursor.foldLeft(pos) { (cursorPos, el) =>
    out.println(s"$cursorPos: $prefix --> $el")
    cursorPos + 1
}

And I'm happy to contribute, if there are more newbie-friendly functions (but progressively harder!) you'd like to delegate to someone I'd love to help

Avasil added some commits Sep 24, 2017

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Sep 26, 2017

@alexandru Well, after some thought I came to the conclusion that I can't use foldLeft for incrementing updated position for batch/cursor because foldLeft is side-effecting (consumes cursor) and dump shouldn't do any side-effects apart from IO

@alexandru alexandru merged commit 7639706 into monix:master Sep 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alexandru alexandru referenced this pull request Nov 14, 2017

Closed

Add Iterant.dump method #434

@alexandru alexandru added this to the 3.0.0 milestone Jan 21, 2018

@Avasil Avasil deleted the Avasil:iterant-dump branch Jan 21, 2018

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