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

Add Iterant.dropWhileWithIndex #512

Merged
merged 5 commits into from Jan 12, 2018

Conversation

Projects
None yet
2 participants
@Avasil
Collaborator

Avasil commented Jan 11, 2018

Fixes #496.

@codecov

This comment has been minimized.

codecov bot commented Jan 11, 2018

Codecov Report

Merging #512 into master will decrease coverage by 6.36%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
- Coverage   90.09%   83.72%   -6.37%     
==========================================
  Files         360      361       +1     
  Lines        9387    10143     +756     
  Branches     1810     1884      +74     
==========================================
+ Hits         8457     8492      +35     
- Misses        930     1651     +721
// Reusable logic for NextCursor / NextBatch branches
@tailrec
def evalCursor(ref: F[Iterant[F, A]], cursor: BatchCursor[A], rest: F[Iterant[F, A]],
stop: F[Unit], dropped: Int, index: Int, continueDropping: Boolean): Iterant[F, A] = {

This comment has been minimized.

@alexandru

alexandru Jan 11, 2018

Member

Personal pet peeve, but I don't like things dangling like that to the right and the codebase is pretty consistent in that regard. This would be better:

    def evalCursor(
      ref: F[Iterant[F, A]], cursor: BatchCursor[A], rest: F[Iterant[F, A]],
      stop: F[Unit], dropped: Int, index: Int, continueDropping: Boolean): Iterant[F, A] = { 

      // ....
try source match {
case ref@Next(item, rest, stop) =>
if (continueDropping && p(item, index)) Suspend(rest.map(loop(index + 1, continueDropping = true)), stop)
else ref

This comment has been minimized.

@alexandru

alexandru Jan 11, 2018

Member

Another personal pet peeve — that line is too long, so something like this maybe?

if (continueDropping && p(item, index)) 
  Suspend(rest.map(loop(index + 1, continueDropping = true)), stop)
else 
  ref 
else if (cursor.hasNext())
Next(elem, ref.map(loop(index, continueDropping = false)), stop)
else
Next(elem, rest.map(loop(index, continueDropping = false)), stop)

This comment has been minimized.

@alexandru

alexandru Jan 11, 2018

Member

So, all of this looks good, however I believe you can simplify it a little - correct me if I'm wrong ...

This law applies:

ref.map(loop(index, continueDropping = false)) <-> ref

Once continueDropping becomes false the predicate p is no longer called and you no longer drop any events, correct? This means that you can just return ref or rest without the .map(loop(...)).

This also means that you can get rid of continueDropping completely. You assume that you have to drop all elements, until the predicate becomes false, at which point you just return whatever you have left, but without the .map(loop(...)).

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 11, 2018

Hey @Avasil, sorry about the style-related comments — one of these days I'll configure a code formatting plugin. I resisted until now due to code formatting plugins not doing a very good job, plus I disagree with some standard practices 😀

But until then I have to annoy people in code reviews.

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Jan 11, 2018

@alexandru No problem, code formatting could be nice because despite doing my best to follow codebase style I don't always get it right and when I actually achieve it - one instinctive ctrl + alt + l in IntelliJ can ruin it :D

Avasil and others added some commits Jan 11, 2018

@alexandru alexandru merged commit 38ed535 into monix:master Jan 12, 2018

1 check passed

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

@Avasil Avasil deleted the Avasil:Iterant#dropWhileIndex branch Jan 21, 2018

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