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.

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

Adds helper to create Shrink using LazyList #627

Merged
merged 1 commit into from
Feb 18, 2020

Conversation

froth
Copy link
Contributor

@froth froth commented Feb 13, 2020

This allows to use Shrink from scala 2.13 without getting deprecation
warnings on scala.collection.immutable.Stream

fixes #626

@mpilquist
Copy link
Member

Awesome! I'd personally prefer this as a method directly on Shrink companion (via the normal pattern of a ShrinkPlatform trait that's mixed in to the companion and defined as an empty trait in 2.12 and prior and a non-empty trait on 2.13.

@ashawley
Copy link
Contributor

Yes, that would be the preference. Though, I don't think it's possible to have both definitions of apply on 2.13 with different override arguments for the function type (T => Stream[T] vs. T => LazyList[T]).

@froth froth force-pushed the lazylistcompat branch 2 times, most recently from b6ba502 to e2f7c09 Compare February 14, 2020 07:34
@froth
Copy link
Contributor Author

froth commented Feb 14, 2020

Though, I don't think it's possible to have both definitions of apply on 2.13 with different override arguments for the function type (T => Stream[T] vs. T => LazyList[T]).

You are right. Although to be honest, I don't get why the compiler can not differentiate between Function1[T, Stream[T]] and Function1[T, LazyList[T]] here.

I followed your suggestions (and am sorry for not looking at the codebase thoroughly before). And I renamed the function from apply to fromLazyList. Again naming is hard, not exactly accurate. Maybe fromLazyListFunction but that seems clunky. Ideas welcome.

@ashawley
Copy link
Contributor

I believe it's because Function1 is implemented with type parameters, as well. There may be a way to overload methods having type parameters with a dummy implicit, but that seems unsavory.

@ashawley
Copy link
Contributor

I suggested it, but maybe fromLazyList is a poor name. Perhaps, withLazyList?

@ashawley
Copy link
Contributor

ashawley commented Feb 14, 2020

If we have a name for the Shrink factory method, instead of using apply, the interface to Shrink has diverged. Should we deprecate apply and add withStream for symmetry sake?

@mpilquist
Copy link
Member

mpilquist commented Feb 14, 2020

Indeed, DummyImplicit can de-conflict the erased overloads if we want it to be called apply:

def apply[T](s: T => LazyList[T])(implicit d: DummyImplicit): Shrink[T]

This allows to use Shrink from scala 2.13 without getting deprecation
warnings on scala.collection.immutable.Stream
@froth
Copy link
Contributor Author

froth commented Feb 14, 2020

Actually DummyImplicit does not do the trick on its own, if I add the apply method with the DummyImplicit I still get a lot of errors like the following:

[error] /home/froth/projects/scalacheck/src/main/scala/org/scalacheck/Shrink.scala:24:49: missing parameter type                                                                                      
[error]   implicit def shrinkAny[T]: Shrink[T] = Shrink(_ => Stream.empty)  

I therefore opted for renaming the factory method to withLazyList.

If we have a name for the Shrink factory method, instead of using apply, the interface to Shrink has diverged. Should we deprecate apply and add withStream for symmetry sake?

I don't have an opinion on that, as I do not know what the further plans for LazyList and scalacheck 2.0 are. I am happy to do whatever you decide ;)

@ashawley
Copy link
Contributor

Thanks for bearing with us and being amenable to our whims. I predict the change is pretty benign, since it's just a factory method for Shrink. It shouldn't paint us in to a corner, and more likely it should be something we can use going forward.

@froth
Copy link
Contributor Author

froth commented Feb 14, 2020

No worries :) I learned something in the process, which is even dearer to me, then getting rid of those annoying warnings.

@ashawley
Copy link
Contributor

I wish there was a way to easily dogfood this new factory in the library. Alternatively, we could add tests, but there aren't many existing example tests for Shrink nor any version-specific tests.

@ashawley ashawley added this to the 1.15.0 milestone Feb 14, 2020
@ashawley
Copy link
Contributor

Thanks again for working on this. We'll wait and see if there are other comments.

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

This looks good to me :)

@SethTisue SethTisue merged commit 400123f into typelevel:master Feb 18, 2020
@SethTisue
Copy link
Member

thanks @froth!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecation warnings because of Stream
5 participants