Cleanups/improvements to protobuf package

[bford]

I'd like to propose the following set of cleanups and other minor improvements (or at least what I see as improvements) for the protobuf package:

• Delete the [S|U]fixed[32|64] types, and instead use struct-tag options to indicate fixed-length encoding for these types. I actually wasn't aware of Go's struct-tag feature when I first designed this package with the kludge for fixed-length-encoded types.
• Simplify the definition of Constructors to 'type Constructors func(reflect.Type) interface{}'. The protobuf package only ever needs to use it to invoke a constructor based on a reflect.Type; it might still be defined based on a map, but this way it can easily be defined in other ways as well.
• Recombine Decode and DecodeWithConstructors into a single function with the signature 'func Decode(buf []byte, structPtr interface{}, options ...interface{}) error'. In this case 'options' is a (by default empty) varargs-list of arbitrary objects that can extensibly represent "decoding options". Initially the only such option that Decode will understand is an instance of the Constructors type, but it may be useful to add other types of options later (via different types), which will then be easy.
• Add a function 'Read(r io.Reader, structPtr interface{}, options ...interface{}) error' that operates like Decode() but decodes from an io.Reader rather than a byte-slice. (Internally, it probably makes sense to implement Decode() in terms of Read().)
• Similarly, add a function 'Write(w io.Writer, structPtr interface{}) error' that operates like Encode() but encodes to an io.Writer rather than a byte-slice.
• Add 'options ...interface{}' arguments to Encode and Write, although at the moment I can't think of any options those functions need to take (but they might want to in the future).
• The Proto-file-generator that @alecthomas added is awesome, but the name GenerateProtobufDefinition seems a bit unnecessarily long (nitpicky I know) and its last couple parameters feel like they should be "options" of some kind that not every caller needs to pass. I propose simplifying the function signature to 'fun GenerateProto(w io.Writer, defs ...interface{})'. The 'defs' vararg-list may contain both struct-types, EnumMap instances, and optionally a GeneratorNamer instance to customize field naming and such.
• If no GeneratorNamer is passed to GenerateProto, the protobuf package internally uses the default namer, which can then be made private instead of public, simplifying the API a bit more. But we should move the documentation about what the default naming scheme is from the DefaultGeneratorNamer godoc into the godoc for GenerateProto.
• Make the following names private, unless anyone knows of a reason anyone outside this package is likely to need to know about or use them: DefaultGeneratorNamer, ProtoField, ProtoFields, ParseTag, Tag*. (Or if there's a reason any of these should remain public, we should add godoc documentation for them.)
• Purely internal: fix encoder.go so that internal encoder methods pass errors back via return instead of panic, eliminating the need for the defer/recover hack in Encode().

@alecthomas and @jackowitzd2 - do these seem reasonable or would any cause problems that you know of (beyond trivial API alignment fixes in dependent code)? Any other suggestions/ideas to improve the protobuf API further?

And can someone start working on these changes in a branch or fork (keeping in mind that this proposal is "tentative" and may further need to change of course)? Perhaps @WEB3-GForce or @jackowitzd2 since you've been playing with encoding/decoding anyway?

Thanks
Bryan

[jackowitz]

I can take on a couple of these changes. I particularly agree that the Read/Write functions would be very useful to have, and the change to Constructors would most likely simplify things as well for the ways I've been using it.

[alecthomas]

Recombine Decode and DecodeWithConstructors into a single function with the signature 'func Decode(buf []byte, structPtr interface{}, options ...interface{}) error'. In this case 'options' is a (by default empty) varargs-list of arbitrary objects that can extensibly represent "decoding options". Initially the only such option that Decode will understand is an instance of the Constructors type, but it may be useful to add other types of options later (via different types), which will then be easy.

With this and GenerateProto I would suggest having an options type with members for each option. So for Decode eg.

type DecodeOptions struct {
  Constructors []Constructor
}

func Decode(buf []byte, ptr interface{}, options *DecodeOptions) {}

This is more idiomatic Go as it is type-safe and discoverable.

Add 'options ...interface{}' arguments to Encode and Write, although at the moment I can't think of any options those functions need to take (but they might want to in the future).

YAGNI?

The Proto-file-generator that @alecthomas added is awesome, but the name GenerateProtobufDefinition seems a bit unnecessarily long (nitpicky I know) and its last couple parameters feel like they should be "options" of some kind that not every caller needs to pass. I propose simplifying the function signature to 'fun GenerateProto(w io.Writer, defs ...interface{})'. The 'defs' vararg-list may contain both struct-types, EnumMap instances, and optionally a GeneratorNamer instance to customize field naming and such.

Similar to above I think this should be an GenerateProtoOptions type.

Make the following names private, unless anyone knows of a reason anyone outside this package is likely to need to know about or use them: DefaultGeneratorNamer, ProtoField, ProtoFields, ParseTag, Tag*. (Or if there's a reason any of these should remain public, we should add godoc documentation for them.)

Yes, good idea. These were left public from when GenerateProto was an external package.

Purely internal: fix encoder.go so that internal encoder methods pass errors back via return instead of panic, eliminating the need for the defer/recover hack in Encode().

This might be nice but I don't think it's necessary. It is idiomatic Go as long as the panic doesn't escape (eg. the JSON package uses panic/recover internally).

[bford]

Thanks Alec for your feedback. Having a DecodeOptions struct is certainly a reasonable approach; the main downside is that the only way to add new options in the long-term is to modify the definition of that struct, and hence risk breaking compatibility (e.g., with code that tries to create a DecodeOptions instance with a positional struct-initializer). One could of course simply state in a comment that code should not depend on the exact membership of struct DecodeOptions, but that feels slightly less than satisfactory. Another very minor nitpick is that callers always have to provide a third option - even if only 'nil' - when they have no options to pass.

The reason I like the 'options ...interface{}' approach is because it's syntactically very clean especially in the no-options case (i.e., you can just call Decode(buf, ptr)), and because of Go's type-switch construct it's very easy and fairly efficient to "parse" an options-list on the call: e.g., within Decode():

var cons Constructors
var ...
for _, opt := range options {
switch o := opt.(type) {
case Constructors: cons = o
case ...
default: panic("Unknown option")
}
}

In the common case when the options list is empty or nearly empty, this should add very little runtime overhead (although I haven't measured exactly "how much") vs just using a struct.

[alecthomas]

Worst case with an explicit options struct is a compile-time error. Worst case (eg. if you remove an option, or change the type of an option) with options...interface{} is a runtime panic. Then there's also the discoverability of an options struct, as previously mentioned. Still, after all is said and done it is your call; I just contributed a bit of code here and there!

[WEB3-GForce]

I'd be happy to help with some of the refactoring. @jackowitzd2 , which tasks items were you planning on tackling?

[jackowitz]

I've got a branch (cleanup) with bullet points 2-5 addressed, although I still need to write some tests for the new functionality. I think the first point may need some discussion about what exactly the tags should be, but if you can take on the generator stuff that'd be great!

[WEB3-GForce]

Sounds good. I'll start working on the generator.

[jackowitz]

Great. I believe most of that code was written by @alecthomas, so maybe see if he has any more input on the changes regarding it.

[WEB3-GForce]

Will do. @alecthomas , are you free sometime over the next few days to meet up to discuss the code?

[jackowitz]

I'm pretty sure he's on a different continent than us :) I just meant seeing where the above discussion goes before jumping in. Sorry for the confusion!

[bford]

Also, to revive this thread - I think the only outstanding/controversial issue was the handling of decoding options. In retrospect there's a third approach that at the moment I think I like even better than either the DecodeOptions struct parameter or the options ...interface{} parameter: namely, just create a 'Decoder' struct type, which contains fields and/or methods for setting decoding options, plus a 'Decode(...)' method whose signature is identical to the current 'Decode(...)' function (but is a method of that Decoder struct rather than a function). The regular Decode(...) function, if we keep it, would then be equivalent to '&Decoder{}.Decode(...)'. Sound reasonable?

[alecthomas]

Sounds like a good plan to me. +1

[bford]

@jackowitzd2 @WEB3-GForce are either of you actively working on this? I'd like to make some progress on it even if not all at once. Thanks.

[jackowitz]

The "cleanup" branch addresses items 2-5 in the initial list in this thread. I'll make sure that the test coverage is good, then open up a pull request.

[jackowitz]

I also missed the resolution of the varags/DecodeOptions question. I'll go back and bring things up to date with the Decoder proposal.

[bford]

Good, thanks Danny.

[WEB3-GForce]

As discussed with you in our meeting on Tuesday, my primary focus is on life insurance at the moment. But, I should have some more time opening up later this week/ this weekend to do this as well.

[nikkolasg]

This issue has a lot of good ideas and we should try to implement them at some point !

[bford]

Agreed :)