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

Remove scaladsl CommandHandler.byState #25655

Closed
patriknw opened this Issue Sep 20, 2018 · 7 comments

Comments

3 participants
@patriknw
Member

patriknw commented Sep 20, 2018

No real value over using plain pattern matching.

This is already done in javadsl, or replaced by something else in javadsl builders.

@johanandren

This comment has been minimized.

Show comment
Hide comment
@johanandren

johanandren Sep 20, 2018

Member

I'll do that right away the ActorContext PR, since I'm anyways deleting stuff.

Member

johanandren commented Sep 20, 2018

I'll do that right away the ActorContext PR, since I'm anyways deleting stuff.

@johanandren johanandren self-assigned this Sep 20, 2018

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Sep 20, 2018

Member

sounds good to get rid of the cruft in preparation for next week

Member

patriknw commented Sep 20, 2018

sounds good to get rid of the cruft in preparation for next week

@renatocaval

This comment has been minimized.

Show comment
Hide comment
@renatocaval

renatocaval Sep 20, 2018

Contributor

You mean there is no real value because the ActorContext is being removed?

Contributor

renatocaval commented Sep 20, 2018

You mean there is no real value because the ActorContext is being removed?

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Sep 20, 2018

Member

that's unrelated

The purpose of byState was to pick a command handler based on the given state, but that can be done with an ordinary function as far as I can see.

Example:

  private def commandHandler: CommandHandler[AccountCommand, AccountEvent, Account] =
    CommandHandler.byState {
      case EmptyAccount      initialHandler
      case OpenedAccount(_)  openedAccountHandler
      case ClosedAccount     closedHandler
    }

vs

  private val commandHandler: CommandHandler[AccountCommand, AccountEvent, Account] =
    (state, cmd) => state match {
      case EmptyAccount      initialHandler(state, cmd)
      case OpenedAccount(_)  openedAccountHandler(state, cmd)
      case ClosedAccount     closedHandler(state, cmd)
    }

or

  private val commandHandler: CommandHandler[AccountCommand, AccountEvent, Account] =
    {
      case (state @ EmptyAccount, cmd)      initialHandler(state, cmd)
      case (state @ OpenedAccount(_), cmd)  openedAccountHandler(state, cmd)
      case (state @ ClosedAccount, cmd)     closedHandler(state, cmd)
    }

plain Scala, nothing new to learn, and more powerful if needed, such as using the substateCommandHander that I sketched in https://github.com/akka/akka/pull/25503/files#diff-33ab95bda9811733ceba30ab72e2090dR93

Member

patriknw commented Sep 20, 2018

that's unrelated

The purpose of byState was to pick a command handler based on the given state, but that can be done with an ordinary function as far as I can see.

Example:

  private def commandHandler: CommandHandler[AccountCommand, AccountEvent, Account] =
    CommandHandler.byState {
      case EmptyAccount      initialHandler
      case OpenedAccount(_)  openedAccountHandler
      case ClosedAccount     closedHandler
    }

vs

  private val commandHandler: CommandHandler[AccountCommand, AccountEvent, Account] =
    (state, cmd) => state match {
      case EmptyAccount      initialHandler(state, cmd)
      case OpenedAccount(_)  openedAccountHandler(state, cmd)
      case ClosedAccount     closedHandler(state, cmd)
    }

or

  private val commandHandler: CommandHandler[AccountCommand, AccountEvent, Account] =
    {
      case (state @ EmptyAccount, cmd)      initialHandler(state, cmd)
      case (state @ OpenedAccount(_), cmd)  openedAccountHandler(state, cmd)
      case (state @ ClosedAccount, cmd)     closedHandler(state, cmd)
    }

plain Scala, nothing new to learn, and more powerful if needed, such as using the substateCommandHander that I sketched in https://github.com/akka/akka/pull/25503/files#diff-33ab95bda9811733ceba30ab72e2090dR93

@johanandren

This comment has been minimized.

Show comment
Hide comment
@johanandren

johanandren Sep 20, 2018

Member

No real need to keep CommandHandler.command either? All it does is _:ing the state parameter.

Member

johanandren commented Sep 20, 2018

No real need to keep CommandHandler.command either? All it does is _:ing the state parameter.

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Sep 20, 2018

Member

let's leave that for now, I think it's fairly common with command handlers that don't care about the state

we had similar discussion about Behaviors.receive and receiveMessage and it's indeed nice with the single param one

Member

patriknw commented Sep 20, 2018

let's leave that for now, I think it's fairly common with command handlers that don't care about the state

we had similar discussion about Behaviors.receive and receiveMessage and it's indeed nice with the single param one

johanandren added a commit to johanandren/akka that referenced this issue Sep 20, 2018

@patriknw patriknw moved this from Backlog to Reviewing in Akka Typed Sep 21, 2018

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Sep 21, 2018

Member

Done in #25654

Member

patriknw commented Sep 21, 2018

Done in #25654

@patriknw patriknw closed this Sep 21, 2018

@patriknw patriknw added this to the 2.5.17 milestone Sep 21, 2018

@patriknw patriknw moved this from Reviewing to Done in Akka Typed Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment