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

ClassTag instead of Class parameter, #1 #194

Merged
merged 1 commit into from Oct 26, 2016

Conversation

patriknw
Copy link
Contributor

@jroper It's possible to use ClassTag for the PersistentEntityRegistry by using abstract type members in PersistentEntity. One drawback to be aware of is that there is no clear compilation error if one forgets to override the type members. See 0127520

  override type Command = Post.BlogCommand
  override type Event = Post.BlogEvent
  override type State = Post.BlogState

There is one more challenge, which I have not solved yet. ReadSide.register. There we have the additional relationship Event <: AggregateEvent[Event]

@@ -32,12 +36,14 @@ object AggregateEventTag {
* same entity will be produced by different event streams and handled by different shards in the read side
* processor, leading to out of order event handling.
*
* @param eventType The type of the event.
* @tparam eventType The type of the event.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be @tparam Event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thx

@@ -49,13 +55,18 @@ object AggregateEventTag {
* same entity will be produced by different event streams and handled by different shards in the read side
* processor, leading to out of order event handling.
*
* @param eventType The type of the event.
* @tparam eventType The type of the event.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again here, should it be @tparam Event?

@patriknw patriknw force-pushed the wip-scaladsl-ClassTag-patriknw branch from 0127520 to 890ee5e Compare October 20, 2016 07:24
@@ -108,6 +109,11 @@ abstract class AbstractPersistentEntityRegistry(system: ActorSystem, injector: I
throw new IllegalArgumentException(s"[${entityClass.getName} must first be registered")
}

override def refFor2[C, P <: PersistentEntity[C, _, _]: ClassTag](entityId: String): PersistentEntityRef[C] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jroper The attempt with abstract type members breaks down completely (as far as I can see) when trying to use it with the PersistentEntityTestDriver. I think that approach is going down a rabbit hole. You can see the problems by testkit-scaladsl/test:compile in https://github.com/patriknw/lagom/tree/wip-scaladsl-ClassTag-NOT-WORKING-patriknw

I was thinking that we could at least try ClassTag for refFor and that would require two parameters like this, kind of repeating the command type. That is maybe not bad, but I'm not sure. You can see usage here: https://github.com/lagom/lagom/pull/194/files#diff-5b233e73c9b93c20d145f9ecd318b512R143

WDYT?

@patriknw patriknw force-pushed the wip-scaladsl-ClassTag-patriknw branch from 890ee5e to d335da3 Compare October 25, 2016 08:46
@patriknw
Copy link
Contributor Author

rebased, let's merge the "ClassTag instead of Class parameter in AggregateEvent" and get back to refFor later.

@patriknw patriknw force-pushed the wip-scaladsl-ClassTag-patriknw branch from d335da3 to fcbdafb Compare October 25, 2016 09:18
@patriknw patriknw merged commit f52b26e into lagom:master Oct 26, 2016
@patriknw patriknw deleted the wip-scaladsl-ClassTag-patriknw branch October 26, 2016 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants