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 a first test applying Scala 3 Union types #667

Merged

Conversation

eloots
Copy link
Contributor

@eloots eloots commented Sep 21, 2023

Linked to #660

I've been looking at various tests in project actor-typed-tests and chose to add a test that is a refactoring of the InteractionPatternsSpec.scala test specification as this test applies a message adapter to handle responses from other actors. Other tests to which this could be applied are:

actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/eventstream/EventStreamSpec.scala
actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/scaladsl/ActorThreadSpec.scala
actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/scaladsl/MessageAdapterSpec.scala
actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/delivery/TestProducerWithAsk.scala
actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/delivery/TestProducerWorkPulling.scala
actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/delivery/TestProducer.scala
actor-typed-tests/src/test/scala/docs/org/apache/pekko/typed/TailChopping.scala
actor-typed-tests/src/test/scala/docs/org/apache/pekko/typed/InteractionPatternsSpec.scala
actor-typed-tests/src/test/scala/docs/org/apache/pekko/typed/Aggregator.scala

Before I would tackle any of the above, I'd like to get feedback first.

What was changed:

  • Use InteractionPatternsSpec as an example for applying Scala 3's Union types to symplify actor code
    • Remove WrappedBackendResponse response message wrapper & backendResponseMapper message adapter
    • Use the union of the actor's public protocol (the Translate message (the only member of the Command ADT) and the possible responses from the backend (messages JobStarted, JobProgress, and JobCompleted): private type CommandAndResponse = Command | Backend.Response
    • Instead of utilising the message adaptor ActorRef in the replyTo field of the message sent to the backend, context.self is used instead
    • The internal (extended) Behavior[CommandAndResponse] is narrowed to Behavior[Command] at creation time
    • The two stage pattern matching on the response is replaced with a direct pattern match on the different possible reponse messages

The diffs between the original and the new version:

136d139
<         private final case class WrappedBackendResponse(response: Backend.Response) extends Command
137a141,142
>         private type CommandAndResponse = Command | Backend.Response
>
139,141c144
<           Behaviors.setup[Command] { context =>
<             val backendResponseMapper: ActorRef[Backend.Response] =
<               context.messageAdapter(rsp => WrappedBackendResponse(rsp))
---
>           Behaviors.setup[CommandAndResponse] { context =>
143,144c146,147
<             def active(inProgress: Map[Int, ActorRef[URI]], count: Int): Behavior[Command] = {
<               Behaviors.receiveMessage[Command] {
---
>             def active(inProgress: Map[Int, ActorRef[URI]], count: Int): Behavior[CommandAndResponse] = {
>               Behaviors.receiveMessage[CommandAndResponse] {
147c150
<                   backend ! Backend.StartTranslationJob(taskId, site, backendResponseMapper)
---
>                   backend ! Backend.StartTranslationJob(taskId, site, context.self)
150,162c153,162
<                 case wrapped: WrappedBackendResponse =>
<                   wrapped.response match {
<                     case Backend.JobStarted(taskId) =>
<                       context.log.info("Started {}", taskId)
<                       Behaviors.same
<                     case Backend.JobProgress(taskId, progress) =>
<                       context.log.info2("Progress {}: {}", taskId, progress)
<                       Behaviors.same
<                     case Backend.JobCompleted(taskId, result) =>
<                       context.log.info2("Completed {}: {}", taskId, result)
<                       inProgress(taskId) ! result
<                       active(inProgress - taskId, count)
<                   }
---
>                 case Backend.JobStarted(taskId) =>
>                   context.log.info("Started {}", taskId)
>                   Behaviors.same
>                 case Backend.JobProgress(taskId, progress) =>
>                   context.log.info2("Progress {}: {}", taskId, progress)
>                   Behaviors.same
>                 case Backend.JobCompleted(taskId, result) =>
>                   context.log.info2("Completed {}: {}", taskId, result)
>                   inProgress(taskId) ! result
>                   active(inProgress - taskId, count)
167c167
<           }
---
>           }.narrow

- Use InteractionPatternsSpec as an example for applying Scala 3's
  Union types to symplify actor code
  - Remove response message wrapper & adapter
  - Use the union of the actor's public protocol (the `Translate`
    message (the only member of the `Command` ADT) and the possible
    responses from the backend (messages `JobStarted`, `JobProgress`,
    and `JobCompleted`): `private type CommandAndResponse = Command | Backend.Response`
  - Instead of utilising the message adaptor `ActorRef` in the `replyTo`
    field of the message sent to the backend, `context.self` is used
    instead
  - The internal (extended) `Behavior[CommandAndResponse]` is narrowed
    to `Behavior[Command]` at creation time

The diffs between the original and the new version:

```bash
136d139
<         private final case class WrappedBackendResponse(response: Backend.Response) extends Command
137a141,142
>         private type CommandAndResponse = Command | Backend.Response
>
139,141c144
<           Behaviors.setup[Command] { context =>
<             val backendResponseMapper: ActorRef[Backend.Response] =
<               context.messageAdapter(rsp => WrappedBackendResponse(rsp))
---
>           Behaviors.setup[CommandAndResponse] { context =>
143,144c146,147
<             def active(inProgress: Map[Int, ActorRef[URI]], count: Int): Behavior[Command] = {
<               Behaviors.receiveMessage[Command] {
---
>             def active(inProgress: Map[Int, ActorRef[URI]], count: Int): Behavior[CommandAndResponse] = {
>               Behaviors.receiveMessage[CommandAndResponse] {
147c150
<                   backend ! Backend.StartTranslationJob(taskId, site, backendResponseMapper)
---
>                   backend ! Backend.StartTranslationJob(taskId, site, context.self)
150,162c153,162
<                 case wrapped: WrappedBackendResponse =>
<                   wrapped.response match {
<                     case Backend.JobStarted(taskId) =>
<                       context.log.info("Started {}", taskId)
<                       Behaviors.same
<                     case Backend.JobProgress(taskId, progress) =>
<                       context.log.info2("Progress {}: {}", taskId, progress)
<                       Behaviors.same
<                     case Backend.JobCompleted(taskId, result) =>
<                       context.log.info2("Completed {}: {}", taskId, result)
<                       inProgress(taskId) ! result
<                       active(inProgress - taskId, count)
<                   }
---
>                 case Backend.JobStarted(taskId) =>
>                   context.log.info("Started {}", taskId)
>                   Behaviors.same
>                 case Backend.JobProgress(taskId, progress) =>
>                   context.log.info2("Progress {}: {}", taskId, progress)
>                   Behaviors.same
>                 case Backend.JobCompleted(taskId, result) =>
>                   context.log.info2("Completed {}: {}", taskId, result)
>                   inProgress(taskId) ! result
>                   active(inProgress - taskId, count)
167c167
<           }
---
>           }.narrow
```
*/

/*
* Copyright (C) 2009-2022 Lightbend Inc. <https://www.lightbend.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this code based on Lightbend code or is it your own? If it is your own, could you use a standard Apache header?

An example - https://github.com/apache/incubator-pekko/blob/main/project/AddMetaInfLicenseFiles.scala#L1-L16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the new file is a modified version of file InteractionPatternsSpec.scala in the Pekko repo, the header is a verbatim copy of the header in that file.

I assume the Lightbend copyright header is there for a reason, unless it was left there in error. In the former case it should be kept in the new file, otherwise it should be removed in both files. Please advise.

Copy link
Member

Choose a reason for hiding this comment

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

If it's totol new, you can let the headerCreate sbt task to create the header.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine. It is correct to keep the headers in this case then.

@eloots
Copy link
Contributor Author

eloots commented Sep 21, 2023

Just wondering as I'm new around here. How does the link validation work or pass? I see that some PR's pass this validation but this PR doesn't even though I think it doesn't change anything wrt. links.
Also, how are the tests run (Pull Request / Check / Tests (pull_request)). I think the failing tests are failing because of the test I added. Just curious about how this works.

@He-Pin
Copy link
Member

He-Pin commented Sep 21, 2023

@eloots It's flasky just ignore it for now, it's not related with your current PR.

@He-Pin
Copy link
Member

He-Pin commented Sep 21, 2023

@eloots I will need more time to understand your great mind, I will do this on this weekend.

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm - this is a variant on an existing test

@eloots
Copy link
Contributor Author

eloots commented Sep 21, 2023

Just wondering; if this gets merged, does this end up in the documentation automatically?
I raised the idea in the linked issue to add this in the docs in separate code tabs (a Scala 3 one next to the existing Scala one).

@pjfanning
Copy link
Contributor

It won't be in docs automatically. We'll need doc changes but in another PR. The idea of a Scala 3 tab won't work because the tabs are configured globally for the project. My preference would be to add a new Scala3 union type .md file and link to it.

@eloots
Copy link
Contributor Author

eloots commented Sep 21, 2023

@He-Pin
If you want to read some more about this: I wrote a blog post in two parts. I forgot to link them in the original issue. You can find them here and here. Hope you enjoy the read!

Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

lgtm

@He-Pin
Copy link
Member

He-Pin commented Sep 25, 2023

@eloots Thank you. Would you like to add a document for it? or we merge this pr first and you can add it later? And your blog post would be a nice addition to show how to use the typed with Scala'3 Union Type.

@pjfanning pjfanning merged commit 966b775 into apache:main Sep 25, 2023
@eloots
Copy link
Contributor Author

eloots commented Sep 26, 2023

@He-Pin

Would you like to add a document for it?

Absolutely! I would like to update the Interaction patterns section and specifically the section about Adapted Response.

Is there any documentation about how to update the documentation 🙂?

It would be nice to add a new illustration about the changed concept of receiving responses using Union types, but I don’t know if the source of the current illustrations is available. If the new illustration could have the same look and feel that would be ideal.

I can add links to the blogposts for sure.

@He-Pin
Copy link
Member

He-Pin commented Sep 26, 2023

@eloots The doc in docs project and just a simple markdown, after you change it ,you can use paradoxBrowse command to check if that's what you want:)

@eloots
Copy link
Contributor Author

eloots commented Sep 26, 2023

Simple markdown. I can do that ;-)

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.

3 participants