Register encode/decode funcs instead of types #699
Conversation
| @@ -38,9 +65,9 @@ type ExtensionObject struct { | |||
| Value interface{} | |||
| } | |||
|
|
|||
| func NewExtensionObject(value interface{}) *ExtensionObject { | |||
| func NewExtensionObject(value interface{}, typeID *ExpandedNodeID) *ExtensionObject { | |||
There was a problem hiding this comment.
It worth making a comment why and what typeID need to be provided i think
There was a problem hiding this comment.
No one should be using this function imo
ua/extension_object.go
Outdated
| df := func(b []byte, vv interface{}) error { | ||
| rv := reflect.ValueOf(vv) | ||
| if rv.Kind() != reflect.Pointer || rv.IsNil() { | ||
| return fmt.Errorf("incorrect type to decode into") |
There was a problem hiding this comment.
I think we need to return what type is expected to avoid people going to the source code to figure that out.
There was a problem hiding this comment.
err := decode(body.Bytes(), &e.Value)
so currently always true; I took this from example in encoding/json golib.
Perhaps simply removing the check altogether is better
|
|
||
| // RegisterExtensionObjectFunc registers a new extension object type using encode and decode functions | ||
| // It panics if the type or the id is already registered. | ||
| func RegisterExtensionObjectFunc(typeID *NodeID, ef encodefunc, df decodefunc) { |
There was a problem hiding this comment.
If this method is public the encodefunc and decodefunc should also be public imo
There was a problem hiding this comment.
Why? Its essentially an interface that you can still adhere to from outside of the package (see read_unknow_node_id_test.go for example). I don't think anyone should even know about the type name, just the function handle
| @@ -28,6 +52,9 @@ const ( | |||
| ExtensionObjectXML = 2 | |||
| ) | |||
|
|
|||
| type encodefunc func(v interface{}) ([]byte, error) | |||
There was a problem hiding this comment.
encodefunc and decodefunc should also be public imo
| e.Value = eotypes.New(typeID) | ||
| if e.Value == nil { | ||
| decode := eotypes.DecodeFunc(typeID) | ||
| if decode == nil { | ||
| debug.Printf("ua: unknown extension object %s", typeID) |
There was a problem hiding this comment.
I think this message is a bit misleading after refactoring. What we now register is funcs iiuc
| mu sync.Mutex | ||
| types map[string]reflect.Type | ||
| ids map[reflect.Type]string | ||
| type FuncRegistry struct { |
There was a problem hiding this comment.
I think it deserves a comment but wrt to functions
There was a problem hiding this comment.
How do you feel about renaming the file from typereg.go -> funcreg.go ?
|
Closing after discussion; this should not be part of this library. |
This change would allow users to register a function to encode or decode extensionobjects they read with a custom function, bypassing the default behaviour that comes from registering a type.
This would enable for example to decode an extensionobject into a type generated at runtime.