-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
chore: Make DeltaOps public in RES CRDTs #32423
Conversation
patriknw
commented
May 20, 2024
- For example when embedding the delta ops in application specific events it is useful to be able to access the underlying ORSet of the delta op, and it's value
* For example when embedding the delta ops in application specific events it is useful to be able to access the underlying ORSet of the delta op, and it's value
@@ -43,8 +43,7 @@ object ORSet { | |||
def underlying: ORSet[A] | |||
} | |||
|
|||
/** INTERNAL API */ | |||
@InternalApi private[akka] final case class AddDeltaOp[A](underlying: ORSet[A]) extends AtomicDeltaOp[A] { | |||
final case class AddDeltaOp[A](underlying: ORSet[A]) extends AtomicDeltaOp[A] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is case classes this means copy, apply, unapply, etc becomes public API/set in stone in addition to the field accessor. Might be fine but worth noting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think we can remove the case
. Could still be useful with unapply
and toString`
@@ -55,7 +62,7 @@ object ORSet { | |||
concatElementsMap(u.elementsMap.asInstanceOf[Map[A, Dot]]), | |||
underlying.vvector.merge(u.vvector))) | |||
case _: AtomicDeltaOp[A @unchecked] => DeltaGroup(Vector(this, that)) | |||
case DeltaGroup(ops) => DeltaGroup(this +: ops) | |||
case g: DeltaGroup[A @unchecked] => DeltaGroup(this +: g.ops) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johanandren I removed the case
For some reason the match exhaustive checks don't work with these custom unapply
. Maybe something with the type parameter? I rewrote them like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, surprised by no MiMa complaints though.
I agree, I was prepared to add mima filter. I don't know if mima is clever enough to understand that previous versions were |