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.scanMap #523

Merged
merged 4 commits into from Jan 15, 2018

Conversation

Projects
None yet
2 participants
@Avasil
Collaborator

Avasil commented Jan 13, 2018

Fixes #519.

@codecov

This comment has been minimized.

codecov bot commented Jan 13, 2018

Codecov Report

Merging #523 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
- Coverage   90.46%   90.44%   -0.02%     
==========================================
  Files         362      362              
  Lines        9609     9610       +1     
  Branches     1826     1820       -6     
==========================================
- Hits         8693     8692       -1     
- Misses        916      918       +2
@alexandru

Good. Added 2 comments on simplifying the tests and maybe some ScalaDoc changes.

Thanks,

@@ -1656,6 +1656,51 @@ sealed abstract class Iterant[F[_], A] extends Product with Serializable {
final def scanEval[S](seed: F[S])(op: (S, A) => F[S])(implicit F: Sync[F]): Iterant[F, S] =
IterantScanEval(self, seed, op)
/** Applies implicitly available `Monoid[B]` to combine elements using
* `Monoid[B]` identity value as the initial state and map every incoming
* element of this `Iterant` with provided function f: A => B.

This comment has been minimized.

@alexandru

alexandru Jan 14, 2018

Member

There's no "identity value" in Monoid, that value is called "empty" or "zero" (depending on the implementation).

Also the provided example defines a Monoid instance on the fly. That's not how this function will get used because people don't define Monoid instances on the fly like that — usually Monoid instances are attached to data types, with tests integrated with cats-laws, etc. What makes a Monoid a Monoid isn't only the interface, but the laws as well, which makes monoid instances expensive. So users that don't have a Monoid[B] instance already will probably not use this function.

I would say something like this:

Given a mapping function that returns a `B` type for which we have 
a [[cats.Monoid]] instance, returns a new stream that folds the incoming
elements of the sources using the provided `Monoid.combine`, with the
initial seed being the `Monoid.empty` value, emitting the generated values
at each step.

Equivalent with [[scan]] applied with the given [[cats.Monoid]], so given 
our `f` mapping function returns a `B`, this law holds:
{{{
  val B = implicitly[Monoid[B]]

  stream.scanMap(f) <-> stream.scan(B.empty)(B.combine)
}}}

Example:
{{{
  // Yields 2, 6, 12, 20, 30, 42
  Iterant[Task].of(1, 2, 3, 4, 5, 6).scanMap(x => x * 2)
}}}

Yeah, docs are harder than code 😛

This comment has been minimized.

@Avasil

Avasil Jan 14, 2018

Collaborator

Regarding "identity value" - I see it often (maybe more in Haskell) but good point anyway because it might only confuse people.

I've changed docs according to your suggestion. :)

test("Iterant.scanMap equivalence to Iterant.scan") { implicit s =>
check1 { (source: Iterant[Coeval, Int]) =>
val scanned1 = source.scanMap(Current(_, 1): State[Int])

This comment has been minimized.

@alexandru

alexandru Jan 14, 2018

Member

I also think these tests are too complicated, because we just defer to scan.

So the only test we need is this scanMap equivalence to scan and IMO you can just operate with Int instead of defining that State.

scan needs more complicated tests like this one, but not scanMap.

Avasil added some commits Jan 14, 2018

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Jan 14, 2018

@alexandru Done!

@alexandru alexandru merged commit 6760150 into monix:master Jan 15, 2018

1 check passed

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

@Avasil Avasil deleted the Avasil:iterant#scanMap branch Jan 21, 2018

@jozic jozic referenced this pull request Feb 25, 2018

Merged

Preparing for 3.0.0-M4 #597

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