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.takeEveryNth operation #510

Merged
merged 11 commits into from Jan 10, 2018

Conversation

Projects
None yet
2 participants
@greenhat
Contributor

greenhat commented Jan 10, 2018

Closes #500

@codecov

This comment has been minimized.

codecov bot commented Jan 10, 2018

Codecov Report

Merging #510 into master will increase coverage by 0.02%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master     #510      +/-   ##
==========================================
+ Coverage   89.99%   90.02%   +0.02%     
==========================================
  Files         355      356       +1     
  Lines        9255     9281      +26     
  Branches     1772     1779       +7     
==========================================
+ Hits         8329     8355      +26     
  Misses        926      926

greenhat added some commits Jan 10, 2018

test("Iterant.takeEveryNth throws on invalid n") { implicit s =>
val source = Iterant[Coeval].nextCursorS(BatchCursor(1,2,3), Coeval.now(Iterant[Coeval].empty[Int]), Coeval.unit)
intercept[IllegalArgumentException] { source.takeEveryNth(0).toListL.value }

This comment has been minimized.

@alexandru

alexandru Jan 10, 2018

Member

Just a note — we need to be precise about what throws the error. Given you have a require in takeEveryNth, then you should delete .toListL.value.

val dummy = DummyException("dummy")
val suffix = Iterant[Coeval].nextCursorS[Int](new ThrowExceptionCursor(dummy), Coeval.now(Iterant[Coeval].empty), Coeval.unit)
val stream = (iter.onErrorIgnore ++ suffix).doOnEarlyStop(Coeval.eval(cancelable.cancel()))
intercept[DummyException] { stream.takeEveryNth(1).toListL.value }

This comment has been minimized.

@alexandru

alexandru Jan 10, 2018

Member

Here you need to be precise about how the error gets thrown too. So the test should be:

assertEquals(stream.takeEveryNth(1).toListL.runTry, Failure(dummy))
@greenhat

This comment has been minimized.

Contributor

greenhat commented Jan 10, 2018

@alexandru Thanks! Fixed the places where the exception is expected.
I'm done with implementation and I'm out of ideas for more tests. Let me know if I miss something.

@greenhat greenhat changed the title from WIP: Add Iterant.takeEveryNth operation to Add Iterant.takeEveryNth operation Jan 10, 2018

require(n > 0, "n must be strictly positive")
if (n == 1) return source

This comment has been minimized.

@alexandru

alexandru Jan 10, 2018

Member

I don't like usage of return, unless it's made for concrete optimization reasons, or in other words it's very low level stuff.

This check should be included before the source match below.

This comment has been minimized.

@greenhat

greenhat Jan 10, 2018

Contributor

@alexandru Totally agree. Just could not find a better place for it. Fixed.

* Implementation for `Iterant#takeEveryNth`
*/
def apply[F[_], A](source: Iterant[F, A], n: Int)
(implicit F: Sync[F]): Iterant[F, A] = {

This comment has been minimized.

@alexandru

alexandru Jan 10, 2018

Member

Don't like this indentation style and the Monix code is pretty consistent on not doing things dangling to the right like that 😀 personal preference, but it's good to be consistent.

Put this signature all on one line, delete the empty line between it and require below and you're done 😀

This comment has been minimized.

@greenhat

greenhat Jan 10, 2018

Contributor

Consistency is the king! Fixed. It's my first PR here, need to learn codebase more. Thanks for the fast and thorough review!

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 10, 2018

This looks good @greenhat, thanks, one last comment about indentation and you're done.

Will then merge as soon as the build is green.

@alexandru alexandru merged commit 43590fe into monix:master Jan 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment