refactor GPA: typed messages#756
Conversation
| // > Set NeedConsensus <- NIL | ||
| if cmi.latestActiveCommittee != nil { | ||
| msgs.AddAll(cmi.suspendCommittee(cmi.latestActiveCommittee)) | ||
| msgs = slices.Concat(msgs, cmi.suspendCommittee(cmi.latestActiveCommittee)) |
There was a problem hiding this comment.
How do you feel about these inline msgs.AddAll(doSometing), slices.Concat(msgs, stateMgr.Input(...)) etc?
Would you agree, that this style is very common in this code and that it decreases readbility? How about changing it to something like this:
res := cmi.suspendCommittee(cmi.latestActiveCommittee)
outmsgs = slices.Concat(msgs, resOutMsgs)
Because I personally loose focus when reading code like: msgs.AddAll(stateMgr.Input(NewInputThingyDone(inputArg))) :D
There was a problem hiding this comment.
Personally I don't find it so distracting, but I'm also not against splitting those lines if it improves readability! Let's do it.
| } | ||
| msg.SetSender(cni.pubKeyAsNodeID(recv.SenderPubKey)) | ||
| cni.sendMessages(cni.chainMgr.Message(msg)) | ||
| cni.sendMessages(cni.chainMgr.Message(gpa.NewMessageIn(cni.pubKeyAsNodeID(recv.SenderPubKey), msg))) |
There was a problem hiding this comment.
I would split that into 3-4 lines :D
| type BasicMessage struct { | ||
| sender NodeID | ||
| recipient NodeID | ||
| type TypedMessageIn[T MessagePayload] struct { |
There was a problem hiding this comment.
Would you like MessageInT more?
There was a problem hiding this comment.
I don't like either 😝
| func (msg *BasicMessage) SetSender(sender NodeID) { | ||
| msg.sender = sender | ||
| type ( | ||
| MessageIn = TypedMessageIn[MessagePayload] |
There was a problem hiding this comment.
While I do see benefit of what you did here, I also see, that TypedMessageIn[SomePayload] is not automatically convertiable to MessageIn. When it was interface it was convertible.
Didn't this aspect create you any problem while refacroting? Because if not - maybe not is not a real drawback.
There was a problem hiding this comment.
Yes, this is why I added the AsTypedMessageIn function, e.g.:
func (cmi *ChainMgr) Message(msg *gpa.MessageIn) []*gpa.MessageOut {
switch msg.Payload.(type) {
case *msgCommitteeLog:
return cmi.handleMsgCommitteeLog(gpa.AsTypedMessageIn[*msgCommitteeLog](msg))
case *msgBlockProduced:
return cmi.handleMsgBlockProduced(gpa.AsTypedMessageIn[*msgBlockProduced](msg))
}
panic(fmt.Errorf("unexpected message %T: %+v", msg, msg))
}
I'm not too happy with the ergonomics. If only Go had better type inference :(
c08084e to
619c630
Compare
619c630 to
0fdb259
Compare
2 participants
No description provided.