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

Drop Scala 2.12 and sbt 1.3 (and 0.13.x leftovers) #10956

Merged
merged 8 commits into from
Aug 30, 2021

Conversation

mkurz
Copy link
Member

@mkurz mkurz commented Aug 19, 2021

No description provided.

@mkurz mkurz added this to the 2.9.0 milestone Aug 19, 2021
@mkurz
Copy link
Member Author

mkurz commented Aug 19, 2021

@SethTisue as you are a Scala expert, is there something we should change here:

trait BodyParser[+A] extends (RequestHeader => Accumulator[ByteString, Either[Result, A]]) {
// "with Any" because we need to prevent 2.12 SAM inference here
self: BodyParser[A] with Any =>

?

Copy link
Member

@marcospereira marcospereira left a comment

Choose a reason for hiding this comment

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

Some minor comments, but it is LGTM, @mkurz!

@SethTisue
Copy link
Member

@SethTisue as you are a Scala expert, is there something we should change here:

This was done by @wsargent in c9d2b33

Dale found that Will wrote a longer explanation of what's going on here, in this GitHub comment: #6691 (comment)

The SAM conversion rules are largely the same in 2.13 and 3 as they were in 2.12, but there are minor differences around the edges, e.g. in the interaction with overloading, but perhaps also in either areas.

So it's conceivable that this is no longer needed somehow, but it's also highly plausible it is still needed and appropriate, so I'd say just leave it unless you're really interested in digging into it.

@mkurz
Copy link
Member Author

mkurz commented Aug 25, 2021

...so I'd say just leave it unless you're really interested in digging into it.

Not interested 😆

@xuwei-k
Copy link
Contributor

xuwei-k commented Aug 28, 2021

as you are a Scala expert, is there something we should change here:

Here is minimized example.

class X1
class X2
class Request
class Result

trait BodyParser1[A] extends (X1 => A)

object ActionBuilder1 {
  def apply[A](parser: BodyParser1[A]): X2 = ???
  def apply[A](block: Request => Result): X2 = ???
}

trait BodyParser2[A] extends (X1 => A) {
  self: Any => // avoid SAM
}

object ActionBuilder2 {
  def apply[A](parser: BodyParser2[A]): X2 = ???
  def apply[A](block: Request => Result): X2 = ???
}

object Main {
  ActionBuilder1(a => ???) // "missing parameter type" error

  ActionBuilder2(a => ???) // no error
}

final def apply[A](bodyParser: BodyParser[A]): ActionBuilder[R, A] = new ActionBuilder[R, A] {
override def parser = bodyParser
protected override def executionContext = self.executionContext
protected override def composeParser[T](bodyParser: BodyParser[T]): BodyParser[T] = self.composeParser(bodyParser)
protected override def composeAction[T](action: Action[T]): Action[T] = self.composeAction(action)
override def invokeBlock[T](request: Request[T], block: R[T] => Future[Result]) = self.invokeBlock(request, block)
}
/**
* Constructs an `Action` with default content.
*
* For example:
* {{{
* val echo = Action { request =>
* Ok("Got request [" + request + "]")
* }
* }}}
*
* @param block the action code
* @return an action
*/
final def apply(block: R[B] => Result): Action[B] = async(block.andThen(Future.successful))

I think with Any is still needed Scala 2.13

@xuwei-k
Copy link
Contributor

xuwei-k commented Aug 28, 2021

BTW with Any does not work with Scala 3.x 😢

-- [E051] Reference Error: A.scala:25:2 ----------------------------------------
25 |  ActionBuilder2(a => ???)
   |  ^^^^^^^^^^^^^^
   |Ambiguous overload. The overloaded alternatives of method apply in object ActionBuilder2 with types
   | [A](block: Request => Result): X2
   | [A](parser: BodyParser2[A]): X2
   |both match arguments (<?> => <?>)

@mkurz
Copy link
Member Author

mkurz commented Aug 30, 2021

Thanks @xuwei-k!

BTW with Any does not work with Scala 3.x 😢

We can have a look at this later when actually adding Scala 3 support for Play.

Copy link
Contributor

@octonato octonato left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @mkurz

@octonato octonato merged commit 9f27380 into playframework:master Aug 30, 2021
@mkurz mkurz deleted the rm_scala212 branch August 30, 2021 07:42
@mkurz
Copy link
Member Author

mkurz commented Aug 30, 2021

Thanks Renato!

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.

None yet

5 participants