Skip to content

Cleanups/improvements to protobuf package #3

@bford

Description

@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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions