Skip to content
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.

Sign up for GitHub

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

Fix Streaming and StreamingT dropWhile functions #856

Merged
merged 4 commits into from
Feb 3, 2016
Merged

Fix Streaming and StreamingT dropWhile functions #856

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6cec86b
Better arbitrary for Int => Boolean
milanshen Feb 3, 2016
4e70028
Use sbt-doctest for some Streaming(T) examples
ceedubs Feb 3, 2016
68d6062
Merge pull request #1 from ceedubs/streaming-doctest
mikejcurry Feb 3, 2016
e180959
Remove a runtime Cons allocation.
milanshen Feb 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/src/main/scala/cats/data/Streaming.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,15 +524,15 @@ sealed abstract class Streaming[A] extends Product with Serializable { lhs =>
*
* For example:
*
* Streaming(1, 2, 3, 4, 5, 6, 7).takeWhile(n => n != 4)
* Streaming(1, 2, 3, 4, 5, 6, 7).dropWhile(n => n != 4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have sbt-doctest in place, we should change these to be compiler-verified!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I submitted mikejcurry#1 to your PR branch

*
* Will result in: Streaming(4, 5, 6, 7)
*/
def dropWhile(f: A => Boolean): Streaming[A] =
this match {
case Empty() => Empty()
case Wait(lt) => Wait(lt.map(_.dropWhile(f)))
case Cons(a, lt) => if (f(a)) Empty() else Cons(a, lt.map(_.dropWhile(f)))
case Cons(a, lt) => if (f(a)) Wait(lt.map(_.dropWhile(f))) else Cons(a, lt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can save an allocation by doing something like this here:
case s @ Cons(a, lt) => if (f(a)) Wait(lt.map(_.dropWhile(f))) else s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot - Thanks! - In general, is there any reason to do s @ and evaluate the expression to s instead of just evaluating to this? Aesthetically I prefer using this, but that is just personal preference - was wondering if there is any real reason I not aware of to prefer one of the other. Eitherways, I'll push through with the s @ ; because the only reason I have any preference for using this is aesthetic, and that likely comes from Java/C++

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To answer your question: there are times when you get a finer-grained type from s @ ... (due to the pattern match ...) and then you would need to use s instead of this because you have better type information. If you don't need the extra type information then using this would be just fine; at that point it's just a stylistic preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool - that makes a lot of sense - and thanks a million for following up!!!

I guess in this instance there would have been no value, as there is no additional type information required, but I can definitely imagine scenarios, where this could be valuable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, totally agree with what @non said. It didn't occur to me that this would work fine in this example. I have no strong preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! - I actually don't have much preference - it's more natural defaults :-). I went with your suggestion anyway.

}

/**
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/scala/cats/data/StreamingT.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ sealed abstract class StreamingT[F[_], A] extends Product with Serializable { lh
*/
def dropWhile(f: A => Boolean)(implicit ev: Functor[F]): StreamingT[F, A] =
this match {
case Cons(a, ft) => if (f(a)) Empty() else Cons(a, ft.map(_.dropWhile(f)))
case Cons(a, ft) => if (f(a)) Wait(ft.map(_.dropWhile(f))) else Cons(a, ft)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as https://github.com/typelevel/cats/pull/856/files#r51714784

case Wait(ft) => Wait(ft.map(_.dropWhile(f)))
case Empty() => Empty()
}
Expand Down
6 changes: 6 additions & 0 deletions laws/src/main/scala/cats/laws/discipline/Arbitrary.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ import org.scalacheck.Arbitrary.{arbitrary => getArbitrary}
*/
object arbitrary extends ArbitraryInstances0 {

// A special function1Arbitrary for testing operations like dropWhile specifically
// in the context of Int => Boolean. Once scalacheck supports better Function1 arbitrary
// instances this can be removed.
implicit def function1Arbitrary: Arbitrary[(Int) => Boolean] =
Arbitrary(getArbitrary[Int].map(x => (input) => input < x))

implicit def constArbitrary[A, B](implicit A: Arbitrary[A]): Arbitrary[Const[A, B]] =
Arbitrary(A.arbitrary.map(Const[A, B]))

Expand Down