Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5012)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 8407043: Second goprotobuf diff

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years ago by awalterschulze
Modified:
11 years ago
Reviewers:
albert.strasheim
Visibility:
Public.

Description

Second goprotobuf diff

Patch Set 1 #

Patch Set 2 : diff -r 9ca3cb2bf046 https://code.google.com/p/goprotobuf/ #

Patch Set 3 : diff -r 9ca3cb2bf046 https://code.google.com/p/goprotobuf/ #

Total comments: 38
Unified diffs Side-by-side diffs Delta from patch set Stats (+10988 lines, -2082 lines) Patch
M CONTRIBUTORS View 1 2 1 chunk +11 lines, -0 lines 0 comments Download
M LICENSE View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M Make.protobuf View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Makefile View 1 2 3 chunks +6 lines, -6 lines 0 comments Download
M README View 1 2 2 chunks +3 lines, -3 lines 0 comments Download
A gogoproto/Makefile View 1 1 chunk +37 lines, -0 lines 0 comments Download
A gogoproto/doc.go View 1 1 chunk +123 lines, -0 lines 4 comments Download
A gogoproto/gogo.proto View 1 1 chunk +36 lines, -0 lines 0 comments Download
A gogoproto/gogo.pb.golden View 1 1 chunk +45 lines, -0 lines 0 comments Download
A last-change View 1 1 chunk +68 lines, -0 lines 2 comments Download
M lib/codereview/codereview.cfg View 1 1 chunk +0 lines, -1 line 0 comments Download
M proto/all_test.go View 1 10 chunks +12 lines, -10 lines 7 comments Download
M proto/clone_test.go View 1 5 chunks +8 lines, -8 lines 0 comments Download
M proto/decode.go View 1 7 chunks +194 lines, -0 lines 0 comments Download
M proto/encode.go View 1 8 chunks +191 lines, -0 lines 0 comments Download
M proto/equal_test.go View 1 1 chunk +1 line, -1 line 0 comments Download
M proto/pointer_unsafe.go View 1 2 7 chunks +128 lines, -0 lines 0 comments Download
M proto/properties.go View 1 7 chunks +190 lines, -138 lines 7 comments Download
M proto/size_test.go View 1 2 chunks +2 lines, -2 lines 0 comments Download
M proto/testdata/golden_test.go View 1 1 chunk +1 line, -1 line 0 comments Download
M proto/testdata/test.proto View 1 2 chunks +4 lines, -4 lines 0 comments Download
M proto/testdata/test.pb.go View 1 27 chunks +115 lines, -1076 lines 0 comments Download
A proto/testdata/test.pb.go.golden View 1 1 chunk +698 lines, -0 lines 0 comments Download
M proto/text.go View 1 3 chunks +27 lines, -1 line 2 comments Download
M proto/text_parser.go View 1 3 chunks +47 lines, -4 lines 4 comments Download
M proto/text_parser_test.go View 1 6 chunks +11 lines, -11 lines 2 comments Download
M proto/text_test.go View 1 2 chunks +2 lines, -2 lines 0 comments Download
M protoc-gen-go/descriptor/Makefile View 1 1 chunk +1 line, -6 lines 0 comments Download
A protoc-gen-go/descriptor/descriptor.proto View 1 1 chunk +621 lines, -0 lines 0 comments Download
M protoc-gen-go/descriptor/descriptor.pb.go View 1 16 chunks +132 lines, -15 lines 0 comments Download
M protoc-gen-go/descriptor/descriptor.pb.golden View 1 38 chunks +499 lines, -371 lines 2 comments Download
A protoc-gen-go/descriptor/gostring.go View 1 1 chunk +185 lines, -0 lines 2 comments Download
M protoc-gen-go/doc.go View 1 1 chunk +2 lines, -2 lines 0 comments Download
M protoc-gen-go/generator/generator.go View 1 23 chunks +180 lines, -169 lines 2 comments Download
M protoc-gen-go/main.go View 1 1 chunk +3 lines, -2 lines 0 comments Download
M protoc-gen-go/plugin/plugin.pb.go View 1 4 chunks +1 line, -22 lines 2 comments Download
A protoc-gen-go/plugin/strings/strings.go View 1 1 chunk +75 lines, -0 lines 0 comments Download
M protoc-gen-go/testdata/Makefile View 1 1 chunk +1 line, -1 line 0 comments Download
M protoc-gen-go/testdata/golden_test.go View 1 1 chunk +1 line, -1 line 0 comments Download
M protoc-gen-go/testdata/my_test/test.pb.go View 1 12 chunks +28 lines, -112 lines 2 comments Download
M protoc-gen-go/testdata/my_test/test.pb.go.golden View 1 12 chunks +28 lines, -112 lines 0 comments Download
A test/custom/custom.go View 1 1 chunk +104 lines, -0 lines 0 comments Download
A test/custom/custom_test.go View 1 1 chunk +41 lines, -0 lines 0 comments Download
A test/doc.go View 1 1 chunk +45 lines, -0 lines 0 comments Download
A test/example.proto View 1 1 chunk +41 lines, -0 lines 0 comments Download
A test/example/Makefile View 1 1 chunk +30 lines, -0 lines 0 comments Download
A test/go/Makefile View 1 1 chunk +31 lines, -0 lines 0 comments Download
A test/goenum.proto View 1 1 chunk +51 lines, -0 lines 0 comments Download
A test/gogo/Makefile View 1 1 chunk +31 lines, -0 lines 0 comments Download
A test/gogoenum.proto View 1 1 chunk +52 lines, -0 lines 0 comments Download
A test/proto_test.go View 1 1 chunk +2606 lines, -0 lines 0 comments Download
A test/protoimpl_test.go View 1 1 chunk +137 lines, -0 lines 0 comments Download
A test/prototext_test.go View 1 1 chunk +2606 lines, -0 lines 0 comments Download
A test/prototextimpl_test.go View 1 1 chunk +126 lines, -0 lines 0 comments Download
A test/thetest.proto View 1 1 chunk +235 lines, -0 lines 0 comments Download
A test/var_test.go View 1 1 chunk +1129 lines, -0 lines 0 comments Download

Messages

Total messages: 7
albert.strasheim
https://codereview.appspot.com/8407043/diff/4001/proto/all_test.go File proto/all_test.go (right): https://codereview.appspot.com/8407043/diff/4001/proto/all_test.go#newcode1357 proto/all_test.go:1357: t.Errorf("got %q, want %q", *m.LastField, "natural goat essence") heh? ...
11 years ago (2013-04-05 21:28:43 UTC) #1
albert.strasheim
https://codereview.appspot.com/8407043/diff/4001/last-change File last-change (right): https://codereview.appspot.com/8407043/diff/4001/last-change#newcode1 last-change:1: # Change list. delete this file https://codereview.appspot.com/8407043/diff/4001/proto/all_test.go File proto/all_test.go ...
11 years ago (2013-04-05 21:34:24 UTC) #2
albert.strasheim
https://codereview.appspot.com/8407043/diff/4001/proto/text.go File proto/text.go (left): https://codereview.appspot.com/8407043/diff/4001/proto/text.go#oldcode188 proto/text.go:188: keep this line https://codereview.appspot.com/8407043/diff/4001/proto/text_parser.go File proto/text_parser.go (right): https://codereview.appspot.com/8407043/diff/4001/proto/text_parser.go#newcode514 proto/text_parser.go:514: ...
11 years ago (2013-04-05 21:40:23 UTC) #3
albert.strasheim
https://codereview.appspot.com/8407043/diff/4001/gogoproto/doc.go File gogoproto/doc.go (right): https://codereview.appspot.com/8407043/diff/4001/gogoproto/doc.go#newcode48 gogoproto/doc.go:48: code.google.com/p/gogoprotobuf/test/gogo by gogoprotobuf this looks a bit strange on ...
11 years ago (2013-04-05 23:14:23 UTC) #4
albert.strasheim
https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/descriptor/descriptor.pb.golden File protoc-gen-go/descriptor/descriptor.pb.golden (left): https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/descriptor/descriptor.pb.golden#oldcode1023 protoc-gen-go/descriptor/descriptor.pb.golden:1023: proto.RegisterEnum("google_protobuf.StreamOptions_TokenUnit", StreamOptions_TokenUnit_name, StreamOptions_TokenUnit_value) what happened to StreamOptions_TokenUnit?
11 years ago (2013-04-06 17:17:32 UTC) #5
albert.strasheim
https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/descriptor/gostring.go File protoc-gen-go/descriptor/gostring.go (right): https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/descriptor/gostring.go#newcode1 protoc-gen-go/descriptor/gostring.go:1: // Extensions for Protocol Buffers to create more go ...
11 years ago (2013-04-06 17:20:35 UTC) #6
awalterschulze
11 years ago (2013-04-08 10:24:41 UTC) #7 
Here are my replies to your comments.

https://codereview.appspot.com/8407043/diff/4001/gogoproto/doc.go
File gogoproto/doc.go (right):

https://codereview.appspot.com/8407043/diff/4001/gogoproto/doc.go#newcode48
gogoproto/doc.go:48: code.google.com/p/gogoprotobuf/test/gogo by gogoprotobuf
What would you prefer?

On 2013/04/05 23:14:23, albert.strasheim wrote:
> this looks a bit strange on http://godoc.org
> 
> http://godoc.org/code.google.com/p/gogoprotobuf/gogoproto

https://codereview.appspot.com/8407043/diff/4001/gogoproto/doc.go#newcode52
gogoproto/doc.go:52: code.google.com/p/gogoprotobuf/test/go by goprotobuf
What would you prefer?

On 2013/04/05 23:14:23, albert.strasheim wrote:
> ditto

https://codereview.appspot.com/8407043/diff/4001/last-change
File last-change (right):

https://codereview.appspot.com/8407043/diff/4001/last-change#newcode1
last-change:1: # Change list.
will do

On 2013/04/05 21:34:24, albert.strasheim wrote:
> delete this file

https://codereview.appspot.com/8407043/diff/4001/proto/all_test.go
File proto/all_test.go (left):

https://codereview.appspot.com/8407043/diff/4001/proto/all_test.go#oldcode118
proto/all_test.go:118: pb.Kind = GoTest_TIME.Enum()
I have not thought about this enough, but maybe another extension is the answer.

On 2013/04/05 21:34:24, albert.strasheim wrote:
> maybe an option to generate short enum names, defaulting to false, so that
these
> changes can go away?

https://codereview.appspot.com/8407043/diff/4001/proto/all_test.go#oldcode1161
proto/all_test.go:1161: &MessageList_Message{
They will get to it, as they have.
I am patient.

On 2013/04/05 21:34:24, albert.strasheim wrote:
> is this gofmt? we can probably contribute this cleanup upstream

https://codereview.appspot.com/8407043/diff/4001/proto/all_test.go
File proto/all_test.go (right):

https://codereview.appspot.com/8407043/diff/4001/proto/all_test.go#newcode1357
proto/all_test.go:1357: t.Errorf("got %q, want %q", *m.LastField, "natural goat
essence")
goat milk

On 2013/04/05 21:28:43, albert.strasheim wrote:
> heh?

https://codereview.appspot.com/8407043/diff/4001/proto/all_test.go#newcode1357
proto/all_test.go:1357: t.Errorf("got %q, want %q", *m.LastField, "natural goat
essence")
goat milk

On 2013/04/05 21:28:43, albert.strasheim wrote:
> heh?

https://codereview.appspot.com/8407043/diff/4001/proto/properties.go
File proto/properties.go (left):

https://codereview.appspot.com/8407043/diff/4001/proto/properties.go#oldcode414
proto/properties.go:414: 
On 2013/04/05 21:34:24, albert.strasheim wrote:
> keep this line

Done.

https://codereview.appspot.com/8407043/diff/4001/proto/properties.go#oldcode518
proto/properties.go:518: 
On 2013/04/05 21:28:43, albert.strasheim wrote:
> add this space back

Done.

https://codereview.appspot.com/8407043/diff/4001/proto/properties.go
File proto/properties.go (right):

https://codereview.appspot.com/8407043/diff/4001/proto/properties.go#newcode291
proto/properties.go:291: if len(p.CustomType) > 0 {
Not without creating smaller functions or copying some code.

On 2013/04/05 21:34:24, albert.strasheim wrote:
> is there a way to structure this change to get the diff smaller?

https://codereview.appspot.com/8407043/diff/4001/proto/text.go
File proto/text.go (left):

https://codereview.appspot.com/8407043/diff/4001/proto/text.go#oldcode188
proto/text.go:188: 
On 2013/04/05 21:40:23, albert.strasheim wrote:
> keep this line

Done.

https://codereview.appspot.com/8407043/diff/4001/proto/text_parser.go
File proto/text_parser.go (right):

https://codereview.appspot.com/8407043/diff/4001/proto/text_parser.go#newcode514
proto/text_parser.go:514: //isDstNil := isNil(dst)
With nullable=false the if is possible and not an error

On 2013/04/05 21:40:23, albert.strasheim wrote:
> have happened here?

https://codereview.appspot.com/8407043/diff/4001/proto/text_parser.go#newcode517
proto/text_parser.go:517: //if !props.Repeated && !isDstNil {
With nullable=false the if is possible and not an error

On 2013/04/05 21:40:23, albert.strasheim wrote:
> and here?

https://codereview.appspot.com/8407043/diff/4001/proto/text_parser_test.go
File proto/text_parser_test.go (right):

https://codereview.appspot.com/8407043/diff/4001/proto/text_parser_test.go#ne...
proto/text_parser_test.go:302: /*{
I rather fixed the test.

On 2013/04/05 21:40:23, albert.strasheim wrote:
> comments above disabled parts of tests explaining what's missing would
probably
> be useful

https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/descriptor/des...
File protoc-gen-go/descriptor/descriptor.pb.golden (left):

https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/descriptor/des...
protoc-gen-go/descriptor/descriptor.pb.golden:1023:
proto.RegisterEnum("google_protobuf.StreamOptions_TokenUnit",
StreamOptions_TokenUnit_name, StreamOptions_TokenUnit_value)
I guess its not in the newer descriptor.proto, but this is a guess

On 2013/04/06 17:17:32, albert.strasheim wrote:
> what happened to StreamOptions_TokenUnit?

https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/descriptor/gos...
File protoc-gen-go/descriptor/gostring.go (right):

https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/descriptor/gos...
protoc-gen-go/descriptor/gostring.go:1: // Extensions for Protocol Buffers to
create more go like structures.
I used it for my description plugin and why shouldn't everyone benefit from this
code?

On 2013/04/06 17:20:35, albert.strasheim wrote:
> is this file needed?

https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/generator/gene...
File protoc-gen-go/generator/generator.go (left):

https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/generator/gene...
protoc-gen-go/generator/generator.go:1093: g.P("type ", ccTypeName, " int32")
On 2013/04/05 21:28:43, albert.strasheim wrote:
> this can go back the way it was

Done.

https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/plugin/plugin....
File protoc-gen-go/plugin/plugin.pb.go (left):

https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/plugin/plugin....
protoc-gen-go/plugin/plugin.pb.go:28: func (m *CodeGeneratorRequest)
GetFileToGenerate() []string {
This will be possible with more pluggable plugins.

On 2013/04/05 21:40:23, albert.strasheim wrote:
> maybe an option to turn getters off too?
> 
> having m == nil work is a maybe a slightly useful feature of getters

https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/testdata/my_te...
File protoc-gen-go/testdata/my_test/test.pb.go (right):

https://codereview.appspot.com/8407043/diff/4001/protoc-gen-go/testdata/my_te...
protoc-gen-go/testdata/my_test/test.pb.go:292: func (m *Request) String() string
        { return proto.CompactTextString(m) }
Unfortunately this is very hard with the new plugin scheme for code generation. 
But it is something I will keep in mind.

On 2013/04/05 21:28:43, albert.strasheim wrote:
> is it possible to make these generate in the same place as before?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b