Fix Streaming and StreamingT dropWhile functions #856
Conversation
Default Int => Boolean in scalacheck just returns a function which always returns the same value (true or false). This means that operations like dropWhile are not tested accurately.
Current coverage is
|
| @@ -524,7 +524,7 @@ 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) | |||
There was a problem hiding this comment.
Now that we have sbt-doctest in place, we should change these to be compiler-verified!
There was a problem hiding this comment.
I submitted mikejcurry#1 to your PR branch
|
Yikes good catch. Thanks @mikejcurry! |
|
This probably warrants a 4.0.1 release :| cc @non @travisbrown |
|
I hope you mean 0.4.1 :-) |
Use sbt-doctest for some Streaming(T) examples
|
@adelbertc oops quite right :) |
| */ | ||
| 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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++
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks! - I actually don't have much preference - it's more natural defaults :-). I went with your suggestion anyway.
|
👍 |
Fix Streaming and StreamingT dropWhile functions
Default
Int => Booleanin scalacheck just returns a function whichalways returns the same value (
trueorfalse). This means thatoperations like
dropWhileare not tested accurately, as whilst the bounds (alltrueand allfalse) are checked, intermediate results which return different values based on input are not tested.This fix creates a temporary
Arbitraryinstance forInt => Booleanand fixes thedropWhileonStreamingandStreamingTwhich were broken, and didn’t show up due to the deficiency in theArbitrary[Int => Boolean]instance provided by scalacheck.Maybe there is a better way to do these instances? This kind of hardcodes that the
Int => Booleanreturned is always just a comparison between theIntvalues, but seems better than alwaystrue/false.