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

Add test for 2.11 and 2.13 version of ExplicitResultType #1167

Merged
merged 7 commits into from Jun 22, 2020

Conversation

mlachkar
Copy link
Collaborator

@mlachkar mlachkar commented Jun 17, 2020

closes #998

@mlachkar mlachkar marked this pull request as ready for review June 18, 2020 12:29
@mlachkar mlachkar requested a review from bjaglin June 18, 2020 12:30
@@ -0,0 +1,56 @@
package tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

so this one doesn't work on scala 2.11?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I forgot this. I will check

Comment on lines 320 to 324
if (productRootClass
.intersect(parents.map(_.typeSymbol).toSet)
.nonEmpty && parents
.map(_.typeSymbol)
.contains(serializableClass)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you extract val parentsClass = parents.map(_.typeSymbol).toSet to make the condition more readable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes.

The code was not matching both Serializable trait in 2.13 and 2.12,
which was leading to cases where got types that are not symplified enought.
in 2.13, only one serializable remains, which was leading to unwanted simplification (ex: Serializable, AnyRef => AnyRef)
Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

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

nice! LGTM (although this is all new to me, so I am not sure my review is worth much!)

mlachkar and others added 4 commits June 21, 2020 17:17
…ypePrinter.scala

Co-authored-by: Brice Jaglin <bjaglin@gmail.com>
…Rename.scala

Co-authored-by: Brice Jaglin <bjaglin@gmail.com>
@mlachkar mlachkar merged commit 83961dd into scalacenter:master Jun 22, 2020
@mlachkar mlachkar deleted the explicitResultType branch October 8, 2020 09:48
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.

Make ExplicitResultTypes work with more Scala versions
2 participants