Skip to content

Conversation

@jhump
Copy link
Contributor

@jhump jhump commented Jan 11, 2018

This is the same as #420, but for the dev branch. This would resolve #385.

@dsnet
Copy link
Member

dsnet commented Jan 12, 2018

Rather than adding a new function, can we change GetExtension to return the raw bytes as a []byte if the ExtensionDesc passed in is "partial"; where we define a partial ExtensionDesc as one where ExtensionDesc.ExtensionType is nil.

@dsnet
Copy link
Member

dsnet commented Jan 12, 2018

It seems to me that this would be a forward compatible change since the current behavior is to panic since GetExtension calls decodeExtension, which calls reflect.New(reflect.TypeOf(ExtensionDesc.ExtensionType)).Elem(), which results in a panic.

@jhump
Copy link
Contributor Author

jhump commented Jan 14, 2018

@dsnet, done. PTAL

delete(extmap, extension.Field)
}

// GetExtension parses and returns the given extension of pb.
Copy link
Member

Choose a reason for hiding this comment

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

// GetExtension retrieves a proto2 extended field from pb.
//
// If the descriptor is type complete (i.e., ExtensionDesc.ExtensionType is non-nil),
// then GetExtension parses the encoded field and returns a Go value of the specified type.
// If the field is not present, then the default value is returned (if one is specified),
// otherwise ErrMissingExtension is reported.
//
// If the descriptor is not type complete (i.e., ExtensionDesc.ExtensionType is nil),
// then GetExtension returns the raw encoded bytes of the field extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jhump
Copy link
Contributor Author

jhump commented Jan 19, 2018

@dsnet: I addressed your last comment. Is this ready to merge now?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants