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

[Vote] How to handle Stream -> LazyList migration #2991

Closed
kailuowang opened this issue Aug 15, 2019 · 13 comments
Closed

[Vote] How to handle Stream -> LazyList migration #2991

kailuowang opened this issue Aug 15, 2019 · 13 comments

Comments

@kailuowang
Copy link
Contributor

Scala deprecated Stream and replace it with LazyList on 2.13, which means that cross compiling libraries have to decide on a strategy on how to handle this migration.

Please vote by thumb up or down the options listed in the two following comments.

Note that we will weight maintainers/code contributors' vote more because this decision has more impact to them.

@kailuowang
Copy link
Contributor Author

Strategy 1: Conflation with type alias, i.e. make the references to the two types interchangeable through type alias.
Cons:
- Reusing the same code for Stream on 2.12- and LazyList on 2.13 is not always sufficient. There are still cases needed for version specific code, e.g. Monad instance.
- For users who continue to use Stream in 2.13, Cats no longer provides any support.
- Cats API might be confusing for users who are not familiar with the conflation approach
Pros:
- Easier for cross compiling users who also chose to conflate the two types.
- Reduce to some degree the complexity introduced by version specific code (as in option 2)

@kailuowang
Copy link
Contributor Author

Strategy 2: all-in version specific - complete the removal of conflation by extending the effort of this PR #2983)
Cons:
- hard-to-discover code duplications in version specific code. Particularly cats.instances.AllInstance, cats.instances.all which are not obvious for people changing those files.
- cross compiling users who are using Stream specific cats API might need to create version specific code as well. (this is a minor con)

Pros:
- Cats API more straightforward
- Possible to add back Stream API (but deprecated) to 2.13 code base

@kailuowang
Copy link
Contributor Author

kailuowang commented Aug 15, 2019

ping maintainers and some stakeholders @LukaJCB @djspiewak @rossabaker @tpolecat @fthomas @johnynek @non @mpilquist @milessabin @ChristopherDavenport @alexandru @SystemFw @larsrh
Please feel free to ignore this if stdlib Stream and LazyList does not concern you.

@travisbrown
Copy link
Contributor

As a footnote, there's some additional discussion here: #2982

It's worth noting that there are popular open source libraries that still use Stream in 2.13, so some users may not have a choice on whether or not they want to alias Stream.

@alexandru
Copy link
Member

Where is Stream used in Cats (except for the instances I guess)?

If it's just a matter of instances, then Cats should provide instances for Stream for as long as it's not removed from the standard library. And it should provide instances for LazyList starting with 2.13.

@travisbrown
Copy link
Contributor

@alexandru Agreed on the instances, but there’s also NonEmptyStream, etc.

@djspiewak
Copy link
Member

I hate to say it, but I like the second one better. It's more work for us, I think, but it's better for everyone downstream and it ultimately leaves us with more flexibility. Even keeping the instances (but @deprecated) actually seems like a pretty decent idea, since people may as well get all the deprecation warnings if they're going to touch Stream.

The first one is definitely ideal for anyone who has type Stream[A] = LazyList[A]. I… can't think of a valid reason why you would want to do that though. Search/replace if you want the global swap.

My 2 cents anyway.

@kailuowang
Copy link
Contributor Author

kailuowang commented Aug 16, 2019

The first one is definitely ideal for anyone who has type Stream[A] = LazyList[A]. I… can't think of a valid reason why you would want to do that though

@djspiewak the main reason I can think of is that using version specific code to handle the two types Stream LazyList with different availability on different scala versions cascades to other code that are not closely related to the two two types. So it's a significantly more effort ( and complexity/duplication spilled over to code unrelated to the two types. The Scala stdlib doesn't have to deal with this because they are on different branches. But maintainers of libraries that cross compile, e.g. Scalacheck, may not have the leasure to put in that extra effort

To me both strategies results in significant ugliness in libraries that cross compile, the depth of the ugliness of strategy 1 is deeper, but it's confined to the two types. The breadth of the ugliness of strategy 2 is wider, and more work for the maintainers, but will cause less confusion to the users.

@rossabaker
Copy link
Member

http4s has that alias to support a private implementation detail. We could easily rip out the alias, and I don't think we're using the instances anyway.

scalacheck doesn't depend on cats, so though that's a deeper use of the alias, I think it's not one affected by this decision.

@kailuowang
Copy link
Contributor Author

I think we have enough votes to go option 2.

@travisbrown
Copy link
Contributor

I can finish up #2983 Monday morning.

@travisbrown
Copy link
Contributor

The pull request is enormous but MiMa and tests are green and it's ready for review: #2983 (comment) /cc @kailuowang

@travisbrown
Copy link
Contributor

I think we can close this now that #2983 is merged.

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

No branches or pull requests

5 participants