code review 676041: Go support for protocol buffers.

Go support for protocol buffers.

Consists of a compiler plugin and the support library, all written in Go.

This is a complete implementation except for:
  - Extensions in the plugin
    - coming soon
    - support is already in the library
  - Services (RPC)
    - needs an external definition to honor before supporting.
  - Insertion points in the plugin
    - may come

[r, 2010-03-21 03:29:23]

Hello rsc@golang.org, dsymonds@golang.org, ken@golang.org (cc:
golang-dev@golang.org),

I'd like you to review this change.

[rsc, 2010-03-21 04:32:55]

LGTM

(changed cc)

http://codereview.appspot.com/676041/diff/1/2
File CONTRIBUTORS (right):

http://codereview.appspot.com/676041/diff/1/2#newcode1
CONTRIBUTORS:1: # This is the official list of people who can contribute
I'd delete this entire comment and just say

# This is a list of contributors to the Go protobuf repository.

http://codereview.appspot.com/676041/diff/1/5
File Makefile (right):

http://codereview.appspot.com/676041/diff/1/5#newcode38
Makefile:38: cd proto; make install
just to avoid possible infinite loops if proto or compiler don't exist

cd proto && make install
cd compiler && make install

http://codereview.appspot.com/676041/diff/1/6
File README (right):

http://codereview.appspot.com/676041/diff/1/6#newcode27
README:27: Extensions are not supported in this release. They will come soon.
Still true?

http://codereview.appspot.com/676041/diff/1/6#newcode37
README:37: # Compile and install the compiler plugin
blank line above this one?

[dsymonds, 2010-03-21 05:10:06]

LGTM

http://codereview.appspot.com/676041/diff/3001/4001
File CONTRIBUTORS (right):

http://codereview.appspot.com/676041/diff/3001/4001#newcode4
CONTRIBUTORS:4: # The AUTHORS file lists the copyright holders; this file
The AUTHORS file is missing from this CL.

http://codereview.appspot.com/676041/diff/3001/4003
File Make.proto (right):

http://codereview.appspot.com/676041/diff/3001/4003#newcode1
Make.proto:1: # Go support for Protocol Buffers - Google's data interchange
format
I'm not sure "Make.proto" is a good name. I was expecting this to be a proto
file.

How about something like Make.inc?

http://codereview.appspot.com/676041/diff/3001/4004
File Makefile (right):

http://codereview.appspot.com/676041/diff/3001/4004#newcode38
Makefile:38: cd proto; make install
The standard way to do this is:
  make -C proto install
  make -C compiler install

http://codereview.appspot.com/676041/diff/3001/4006
File compiler/Makefile (right):

http://codereview.appspot.com/676041/diff/3001/4006#newcode44
compiler/Makefile:44: cd testdata && make test
make -C testdata test

http://codereview.appspot.com/676041/diff/3001/4035
File proto/text_parser.go (right):

http://codereview.appspot.com/676041/diff/3001/4035#newcode36
proto/text_parser.go:36: //     - message sets, groups.
s/message sets, //

[r2, 2010-03-21 05:30:40]

On Mar 20, 2010, at 10:10 PM, dsymonds@golang.org wrote:

> LGTM
> 
> 
> http://codereview.appspot.com/676041/diff/3001/4001
> File CONTRIBUTORS (right):
> 
> http://codereview.appspot.com/676041/diff/3001/4001#newcode4
> CONTRIBUTORS:4: # The AUTHORS file lists the copyright holders; this
> file
> The AUTHORS file is missing from this CL.

The comment is a dreg. this one is going with protobuf's license. simplified.

> http://codereview.appspot.com/676041/diff/3001/4003
> File Make.proto (right):
> 
> http://codereview.appspot.com/676041/diff/3001/4003#newcode1
> Make.proto:1: # Go support for Protocol Buffers - Google's data
> interchange format
> I'm not sure "Make.proto" is a good name. I was expecting this to be a
> proto file.
> 
> How about something like Make.inc?

I changed it Make.protobuf.

> http://codereview.appspot.com/676041/diff/3001/4004
> File Makefile (right):
> 
> http://codereview.appspot.com/676041/diff/3001/4004#newcode38
> Makefile:38: cd proto; make install
> The standard way to do this is:
>  make -C proto install
>  make -C compiler install

Standard?  Never heard of it, but I believe you. Still, I made them all
	cd XX && make install
which is clear, if non-standard.

> http://codereview.appspot.com/676041/diff/3001/4006
> File compiler/Makefile (right):
> 
> http://codereview.appspot.com/676041/diff/3001/4006#newcode44
> compiler/Makefile:44: cd testdata && make test
> make -C testdata test
> 
> http://codereview.appspot.com/676041/diff/3001/4035
> File proto/text_parser.go (right):
> 
> http://codereview.appspot.com/676041/diff/3001/4035#newcode36
> proto/text_parser.go:36: //     - message sets, groups.
> s/message sets, //

Oops.
> 
> http://codereview.appspot.com/676041/show

[r2, 2010-03-21 05:31:00]

On Mar 20, 2010, at 9:32 PM, rsc@golang.org wrote:

> LGTM
> 
> (changed cc)
> 
> 
> http://codereview.appspot.com/676041/diff/1/2
> File CONTRIBUTORS (right):
> 
> http://codereview.appspot.com/676041/diff/1/2#newcode1
> CONTRIBUTORS:1: # This is the official list of people who can contribute
> I'd delete this entire comment and just say
> 
> # This is a list of contributors to the Go protobuf repository.

Done.

> 
> http://codereview.appspot.com/676041/diff/1/5
> File Makefile (right):
> 
> http://codereview.appspot.com/676041/diff/1/5#newcode38
> Makefile:38: cd proto; make install
> just to avoid possible infinite loops if proto or compiler don't exist

Done.

> 
> cd proto && make install
> cd compiler && make install
> 
> http://codereview.appspot.com/676041/diff/1/6
> File README (right):
> 
> http://codereview.appspot.com/676041/diff/1/6#newcode27
> README:27: Extensions are not supported in this release. They will come
> soon.
> Still true?

Yes, although I clarified the sentence.

> http://codereview.appspot.com/676041/diff/1/6#newcode37
> README:37: # Compile and install the compiler plugin
> blank line above this one?

Done.

> 
> http://codereview.appspot.com/676041/show

[r, 2010-03-21 05:32:37]

*** Submitted as
http://code.google.com/p/goprotobuf/source/detail?r=2fd7ffde48b1 ***

Go support for protocol buffers.

Consists of a compiler plugin and the support library, all written in Go.

This is a complete implementation except for:
  - Extensions in the plugin
    - coming soon
    - support is already in the library
  - Services (RPC)
    - needs an external definition to honor before supporting.
  - Insertion points in the plugin
    - may come

R=rsc, dsymonds1, ken2
CC=golang-dev
http://codereview.appspot.com/676041