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

Make the AbstractBehavior builder mutable #26309

Merged
merged 5 commits into from Feb 12, 2019

Conversation

Projects
None yet
4 participants
@johanandren
Copy link
Member

johanandren commented Jan 30, 2019

Fixes #26260

I think we should leave the builder used by akka.actor.typed.javadsl.Behaviors immutable since it is more FP:y.

Couldn't find a single sample where it felt natural/needed to use the style of separate blocks, maybe we should add a sample specifically for that?

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Jan 30, 2019

Test FAILed.

@johanandren

This comment has been minimized.

Copy link
Member Author

johanandren commented Jan 31, 2019

Unrelated failure #26230

@johanandren

This comment has been minimized.

Copy link
Member Author

johanandren commented Jan 31, 2019

PLS BUILD

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Jan 31, 2019

Test PASSed.

@patriknw
Copy link
Member

patriknw left a comment

looking good, I agree with the mutable vs immutable
a few naming questions

@@ -32,7 +32,7 @@ class ReceiveBuilder[T] private (
* @param type type of message to match
* @param handler action to apply if the type matches
* @tparam M type of message to match
* @return a new behavior with the specified handling appended
* @return this behavior builder
*/
def onMessage[M <: T](`type`: Class[M], handler: Function[M, Behavior[T]]): ReceiveBuilder[T] =

This comment has been minimized.

@patriknw

patriknw Jan 31, 2019

Member

should we align the naming?
In CommandHandlerBuilder it's matchCommand. Should this be matchMessage, or should it be onCommand in CommandHandlerBuilder?

This comment has been minimized.

@johanandren

johanandren Jan 31, 2019

Author Member

I think we should align. The only argument I can find either way for selecting which name is that onMessage is shorter than matchMessage.

This comment has been minimized.

@patriknw

patriknw Jan 31, 2019

Member

perhaps also because pattern matching is not a thing

This comment has been minimized.

@renatocaval

renatocaval Jan 31, 2019

Contributor

I prefer on then match. Just because it expresses better the fact that we are reacting to the arrival of something.

The match variant aligns better to what happens behind, ie: we lookup for a handler that matches the criteria. We could even argue that match is leaking the implementation behind while on is descriptive to what should happen.

Making it short: +1 for onMessage, onCommand and onEvent.

This comment has been minimized.

@patriknw

patriknw Jan 31, 2019

Member

decided then!

This comment has been minimized.

@johanandren

johanandren Jan 31, 2019

Author Member

Created an issue to do that separately #26313

Show resolved Hide resolved ...ctor-typed/src/main/scala/akka/actor/typed/javadsl/BehaviorBuilder.scala
Show resolved Hide resolved ...actor-typed/src/main/scala/akka/actor/typed/javadsl/ReceiveBuilder.scala Outdated
@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Jan 31, 2019

Test FAILed.

A bit of rework of the Java builders:
 * onAnyMessage added
 * use the japi SAMs throughout in the APIs
 * avoid wrapping the japi functions in Scala functions for the most common cases
 * more Java test coverage
@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Jan 31, 2019

Test PASSed.

Show resolved Hide resolved ...actor-typed/src/main/scala/akka/actor/typed/javadsl/ReceiveBuilder.scala
* @param handler action to apply when the type matches
* @return a new behavior with the specified handling appended
*/
def onSignalUnchecked[M <: Signal](`type`: Class[_ <: Signal], handler: Function[M, Behavior[T]]): ReceiveBuilder[T] =

This comment has been minimized.

@patriknw

patriknw Jan 31, 2019

Member

right, signals are never generic types

@patriknw
Copy link
Member

patriknw left a comment

LGTM, with one doc nitpick

@akka-ci akka-ci added the validating label Jan 31, 2019

@akka-ci akka-ci added tested and removed tested validating labels Jan 31, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Jan 31, 2019

Test PASSed.

private val signalHandlers: List[ReceiveBuilder.Case[T, Signal]]
final class ReceiveBuilder[T] private (
private var messageHandlers: List[ReceiveBuilder.Case[T, T]],
private var signalHandlers: List[ReceiveBuilder.Case[T, Signal]]
) {

This comment has been minimized.

@patriknw

patriknw Feb 1, 2019

Member

do you thing we should add orElse? command and event builders have that
(separate PR)

This comment has been minimized.

@johanandren

johanandren Feb 1, 2019

Author Member

I think it is less useful here than in typed persistence. Could be nice for consistency though.

This comment has been minimized.

@renatocaval

renatocaval Feb 13, 2019

Contributor

Coming late to the party, but...

I don't think it's less useful. You can define different ReceiveBuilder and compose then differently depending on each state/behavior your are. That was a common pattern in untyped, no? Combining different Receive PF.

Or am I missing something here?

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Feb 11, 2019

Test PASSed.

@patriknw patriknw merged commit 8fabb73 into akka:master Feb 12, 2019

3 checks passed

Jenkins PR Validation Test PASSed. 2943 tests run, 19 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.