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 msg to json #105

Merged
merged 4 commits into from
Jul 23, 2017
Merged

100 msg to json #105

merged 4 commits into from
Jul 23, 2017

Conversation

bfbachmann
Copy link
Member

Related Issue

#100

Description

  • This change moves our message encoding and decoding to JSON. Specifically it changes messages so there is now a concrete Message type (rather than the interface we used before) that holds the payload type and the payload in byte slice form. This allows us to correctly decode the payload when a message is received.
  • Payloads can be one of Push, Request, and Response.
  • The Read function will return a MessagePayload (essentially just an interface that wraps the Request, Response, and Push concrete types).
  • Callers of Read can switch on the returned payload type to retrieve its underlying concrete type.

WIKI Updates

Todos

None. Cuz i get werk done.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 71.56% when pulling 7e56a14 on 100-msg-to-json into 0c6862b on dev.

Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

Couple minor suggestions

msg/message.go Outdated
// ProtocolError is an error that occured during a request.
type ProtocolError struct {
Code ErrorCode
Message string
Code ErrorCode `json:"Code"`
Copy link
Member

Choose a reason for hiding this comment

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

If the json struct tag is the same as the struct field name, then it can be omitted. I'd say either remove the struct tags or make them camelCase like typical JSON

msg/message.go Outdated
ID string
ResourceType ResourceType
Params map[string]interface{}
ID string `json:"ID"`
Copy link
Member

Choose a reason for hiding this comment

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

same point applies in these structs for json tags

peer/peer.go Outdated
@@ -223,17 +223,18 @@ func (p *Peer) Dispatch() {
}
errCount = 0

switch message.(type) {
switch payload.(type) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I like how you avoided needing changes outside message package

@@ -316,15 +315,19 @@ func MaintainConnections() {
// PeerInfoHandler will handle the response to a PeerInfo request by attempting
// to establish connections with all new peers in the given response Resource.
func PeerInfoHandler(res *msg.Response) {
peers := res.Resource.([]string)
peers := res.Resource.([]interface{})
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because when arrays are unmarshalled from JSON they become []interface{} in Go.

Copy link
Member

Choose a reason for hiding this comment

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

Right but we're type casting them. Why can't we type cast it directly to []string like before rather than type casting each element individually?

peer/peer.go Outdated

for !receivedAddr || !sentAddr {
message, err := msg.Read(c)
payload, err := msg.Read(c)
Copy link
Member

Choose a reason for hiding this comment

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

Minor thing but I think calling this message still makes sense. Technically we're extracting the "payload" from the "message" in the message package but from the context of peer this is a message.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 71.56% when pulling 6ce6036 on 100-msg-to-json into 0c6862b on dev.

@bfbachmann bfbachmann merged commit 086039b into dev Jul 23, 2017
@jordanschalm jordanschalm deleted the 100-msg-to-json branch August 30, 2017 21:10
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.

3 participants