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

Fix all compiler warnings on Scala 2.13 #2893

Closed
kailuowang opened this issue Jun 18, 2019 · 4 comments · Fixed by #2914
Closed

Fix all compiler warnings on Scala 2.13 #2893

kailuowang opened this issue Jun 18, 2019 · 4 comments · Fixed by #2914
Labels
good first issue Issues that are easier to take on for first time contributors help wanted
Milestone

Comments

@kailuowang
Copy link
Contributor

kailuowang commented Jun 18, 2019

Fatal-warning was temporarily disabled on Scala 2.13. Removing them might require breaking changes, hence we'd better do it before 2.0-RC1
This might be too much as a single task. We should probably achieve this with multiple PRs.
If anyone is interested, please feel free to choose a smaller scope to work on and announce it here. So that we can parallelize.

@kailuowang kailuowang added this to the 2.0.0-RC1 milestone Jun 18, 2019
@poslegm
Copy link

poslegm commented Jun 18, 2019

Should cats 2.0 for Scala 2.13 contains instances for Stream? It's deprecated and replaced by LazyList.

@kailuowang
Copy link
Contributor Author

kailuowang commented Jun 18, 2019

@poslegm thanks for looking at this. I would say no, cats 2.0 code base should not have any deprecation warnings. Thus we need to move all the Stream instances to the scala version specific source folders /core/src/main/scala-2.12-/cats/. and probably need a blank StreamInstances trait in the 2.13+ folder For the first PR I don't think we have to add the LazyList instance yet, that can be a different PR.

Stream is also used in laws/ExhausiveCheck, in which case maintaining cross build might be hard. I propose we replace that with a List which was discussed during the PR introducing ExhausiveCheck. We ended didn't switch to List at that time due to lack of benefit but now we do. That could be a separate PR if you want to control the scope of the one you are working on.

Update: I ended up creating the PR for replacing Stream in ExhausiveCheck. Also added Scala version specific folders for tests in the build. It is necessary when we move all the Stream instances and related tests to version specific folders
#2896

Thanks!!!

@kailuowang
Copy link
Contributor Author

@poslegm I realized that we probably still want to keep Stream instances in 2.13 but deprecate them only on 2.13, this will require duplication the code in two scala version specific folders. I think I am going to take stab myself.

@kailuowang
Copy link
Contributor Author

Update Stream deprecations are resolved by #2904

@kailuowang kailuowang added the good first issue Issues that are easier to take on for first time contributors label Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are easier to take on for first time contributors help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants