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

100 clone generate rsp #116

Open
wants to merge 7 commits into
base: v4
Choose a base branch
from
Open

100 clone generate rsp #116

wants to merge 7 commits into from

Conversation

MengyangHe1
Copy link

No description provided.

@MengyangHe1 MengyangHe1 requested a review from syifan June 12, 2024 13:57

// GenerateRsp generates response to originral translation request
func (r *TranslationReq) GenerateRsp(page Page) sim.Rsp {
rsp := TranslationRspBuilder{}.WithSrc(r.Dst).WithDst(r.Src).WithRspTo(r.ID).WithPage(page).Build()
Copy link
Contributor

Choose a reason for hiding this comment

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

Write in multiple lines to avoid long lines.

@@ -18,6 +18,11 @@ func (m *TrafficMsg) Meta() *sim.MsgMeta {
return &m.MsgMeta
}

// Clone returns cloned TrafficMsg
func (m *TrafficMsg) Clone() sim.Msg {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to really clone it rather than just return the pointer.

}

func (p *PingMsg) GenerateRsp() sim.Rsp {
rsp := &PingRsp{}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should need a new ID, right?

}

func (p *PingRsp) GetRspTo() string {
return strconv.Itoa(p.SeqID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the request have an ID? If so, we need to get the request ID. If not, we need to add the ID to the request.

@@ -3,6 +3,8 @@ package sim
// A Msg is a piece of information that is transferred between components.
type Msg interface {
Meta() *MsgMeta
Clone() Msg
// GenerateRsp() Rsp
Copy link
Contributor

Choose a reason for hiding this comment

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

It is good that you do not add a GenerateRsp as a required field. A recommendation is that you can have two extra interfaces, Request and Response. The Request can have GenerateRsp method and the Response can have the GetRspTo method.

@@ -72,6 +72,10 @@ func (m *sampleMsg) Meta() *MsgMeta {
return &m.MsgMeta
}

func (m *sampleMsg) Clone() Msg {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, if it is not difficult to clone the message, why not using the code below?

func (c *ControlMsg) Clone() Msg {
	cloneMsg := *c
	cloneMsg.ID = GetIDGenerator().Generate()

	return &cloneMsg
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants