Drop types declaration option #250

Serabe · 2017-01-10T09:30:36Z

At src-d, we are using our Go code as source of truth for the rest of the platform. For this, we started writing proteus, a tool that creates proto files from our Go source.

Due to this, we needed to remove type declarations coming from both protobuf messages and enums. These options seem useful for other people and projects (like kubernetes/gengo), so we are opening this PR in case you find this feature helpful.

Thank you!

awalterschulze · 2017-01-10T09:45:20Z

Really cool project.
I am wondering if I understand one of your future ideas correctly.
But is it possible to generate the gogoprotobuf.proto file from the gotypes, like kubernetes has done, but has not exposed as a separate project?
Would it be possible then to translate thrift definitions to proto definitions, by generated the go code from thrift and then generated the proto file from the gotypes?

Really cool idea about omitting the generated types. I'll just have to think about this concept a little.

Also can't you use the plugins: populate, equal and testgen to generate those tests, that you manually wrote?

awalterschulze · 2017-01-10T09:46:28Z

To fix the tests you could rather use gogofast_out is possible?

awalterschulze · 2017-01-10T10:45:49Z

@smarterclayton do think this feature will help kubertnetes?

awalterschulze · 2017-01-17T16:34:28Z

test/enum_drop_type_declaration/Makefile

+regenerate:
+	go install github.com/gogo/protobuf/protoc-gen-gogo
+	go install github.com/gogo/protobuf/protoc-gen-gofast
+	protoc --gofast_out=. --proto_path=../../../../../:../../protobuf/:. thetest.proto


protoc-min-version --version="3.0.0" --gogo_out=. --proto_path=../../../../../:../../protobuf/:. thetest.proto

Add two options, one for messages and one for enums, to drop type declarations. In messages, it also drops getters and setters. In enums, it also drops the generated constants.

Serabe · 2017-01-18T08:27:09Z

Fixed and rebased to clean the history

Serabe · 2017-01-18T09:00:25Z

Travis is not picking up the PR, can you manually start the build? Thank you!

awalterschulze · 2017-01-18T10:21:05Z

There are still some gofast_out commands left
They need to be replaced with

protoc-min-version --version="3.0.0" --gogo_out=. ...

or

protoc-min-version --version="3.0.0" --gogofast_out=. ...

awalterschulze · 2017-01-18T10:21:16Z

I just restarted the build

Serabe · 2017-01-18T11:18:48Z

Fixed, thank you!

Serabe · 2017-01-18T12:17:08Z

Build is passing, let me know if you want me to do a rebase.

Thank you!

awalterschulze · 2017-01-18T12:19:57Z

gogoproto/helper.go


 func HasGoGetters(file *google_protobuf.FileDescriptorProto, message *google_protobuf.DescriptorProto) bool {
-	return proto.GetBoolExtension(message.Options, E_GoprotoGetters, proto.GetBoolExtension(file.Options, E_GoprotoGettersAll, true))
+	return proto.GetBoolExtension(message.Options, E_GoprotoGetters, proto.GetBoolExtension(file.Options, E_GoprotoGettersAll, true)) && !IsDropTypeDeclaration(message)


Why?
I mean if someone doesn't want gogetters they can turn them off, but otherwise there is a good chance they will still work?

The reason is that in protoc-gen-gogo/generator/generator.go the information needed to create the getters is not worked out unless the struct is created. I can try and refactor that part if you are ok with it.

I think being able to turn the getters on or off will useful.
But its unfortunate that this has to complicate the code.
Please go ahead and refactor this, thank you very much.

awalterschulze · 2017-01-18T12:24:07Z

protoc-gen-gogo/generator/generator.go

-	g.PrintComments(message.path)
-	g.P("type ", ccTypeName, " struct {")
-	g.In()
+	if !gogoproto.IsDropTypeDeclaration(message.DescriptorProto) {


github is really struggling with this diff, I assume you only added this if

If + GoFmt. Check here

awalterschulze · 2017-01-18T12:24:13Z

protoc-gen-gogo/generator/generator.go


-		g.P(name, " ", ccTypeName, " = ", e.Number)
-		g.file.addExport(enum, constOrVarSymbol{name, "const", ccTypeName})
+	if !gogoproto.IsEnumDropTypeDeclaration(enum.EnumDescriptorProto) {


github is really struggling with this diff, I assume you only added this if

If + GoFmt. Check here

awalterschulze · 2017-01-18T12:24:57Z

test/drop_type_declaration/Makefile

@@ -0,0 +1,4 @@
+regenerate:
+	go install github.com/gogo/protobuf/protoc-gen-gogo
+	go install github.com/gogo/protobuf/protoc-gen-gofast


No need to install gofast anymore

awalterschulze · 2017-01-18T12:25:26Z

test/drop_type_declaration/models.go

+	return true
+}
+
+func (d *Dropped) GetName() string {


The generated getters would probably be fine here

awalterschulze · 2017-01-18T12:26:44Z

test/drop_type_declaration/thetest.proto

@@ -0,0 +1,27 @@
+syntax = "proto3";


Could we call this file something else and not thetest.proto as well?
How about drop.proto

awalterschulze · 2017-01-18T12:27:51Z

Since everything is working I am going to start becoming a little bit more nit picky.

Serabe · 2017-01-18T12:31:16Z

Great! I keep things in two different commits to ease the review.

Thank you!

awalterschulze · 2017-01-18T13:02:09Z

I am looking at everything everytime, so don't worry about the professionalism of the commits too much. As long as the end result looks good I'm happy.

Serabe · 2017-01-18T13:08:09Z

I:

Renamed thetest.proto to drop.proto and enumdrop.proto. Generated files were removed and regenerated.
Remove gofast install instruction from Makefiles.
Provided a gist with the patch for protoc-gen-gogo/generator/generator.go.

I'm currently refactoring generator.go so drop options and getters are fully independent.

Thank you!

This commit extract a few variable initializations that was being done at the same time of generating the type for a given message. This allow for the Go Getters option to be independent from the drop type declaration one

Serabe · 2017-01-19T09:41:22Z

I've exrtracted some variable declaration out of the part of the code that generated the type definition for a given message.

I think I have extracted the minimal amount of code needed for go getters to be independent from drop type declaration.

I think this completes your first review.

Thank you!

awalterschulze · 2017-01-19T17:12:59Z

protoc-gen-gogo/generator/generator.go

 	}

-	for i, field := range message.Field {
-		// Allocate the getter and the field at the same time so name


Why is this comment deleted?

Fixed in next push. I intended to fix it, but I rather do that if I have time in the future in a different PR.

Ok thats fine, then just bring back the comment :)

This is coming all the way from golang/protobuf so we want to fix it there and then pull that fix in here.

awalterschulze · 2017-01-19T17:14:06Z

extensions.md

 <tr><td><a href="http://godoc.org/github.com/gogo/protobuf/plugin/marshalto"> unsafe_marshaler</a> </td><td> Message </td><td> bool </td><td> if true, a Marshal and MarshalTo method is generated for the specific message. The generated code uses the unsafe package. </td><td> false</td></tr>
 <tr><td><a href="http://godoc.org/github.com/gogo/protobuf/plugin/unmarshal">unsafe_unmarshaler</a></td><td> Message </td><td> bool </td><td> if true, an Unmarshal method is generated for the specific message. The generated code uses the unsafe package. </td><td> false</td></tr>
 <tr><td><a href="http://godoc.org/github.com/gogo/protobuf/plugin/marshalto">stable_marshaler</a></td><td> Message </td><td> bool </td><td> if true, a Marshal and MarshalTo method is generated for the specific message, but unlike marshaler the output is guaranteed to be deterministic, at the sacrifice of some speed</td><td> false </td></tr>
+<tr><td>drop_type_declaration (beta)</td><td> Message </td><td> bool </td><td> if true, type declaration of messages are excluded from output. </td><td> false </td></tr>


What about 'typedecl' with default true

awalterschulze · 2017-01-19T17:14:21Z

extensions.md

 <tr><td><a href="http://godoc.org/github.com/gogo/protobuf/gogoproto"> castkey </a> (beta) </td><td> Field </td><td> string </td><td> Changes the generated fieldtype for a map key.  All generated code assumes that this type is castable to the protocol buffer field type.  Only supported on maps. </td><td> goprotobuf field type </td></tr>
 <tr><td><a href="http://godoc.org/github.com/gogo/protobuf/gogoproto"> castvalue </a> (beta) </td><td> Field </td><td> string </td><td> Changes the generated fieldtype for a map value.  All generated code assumes that this type is castable to the protocol buffer field type.  Only supported on maps. </td><td> goprotobuf field type </td></tr>
 <tr><td>enum_customname  (beta)</td><td> Enum </td><td> string </td><td>Sets the type name of an enum. If goproto_enum_prefix is enabled, this value will be used as a prefix when generating enum values.</td><td>goprotobuf enum type name. Helps with golint issues.</td></tr>
+<tr><td>enum_drop_type_declaration (beta)</td><td> Enum </td><td> bool </td><td> if true, type declaration of enums are excluded from output. </td><td> false </td></tr>


What about 'enumdecl' with default true

awalterschulze · 2017-01-19T17:14:44Z

Looks really good :)

smarterclayton · 2017-01-19T17:37:52Z

Yes this would help us - we currently perform a Go AST traversal of the generated types to remove it and this would remove the need for us having to do that.

awalterschulze · 2017-01-19T21:04:27Z

Thats great to hear. I just remembered, can you also add Fileoptions, _all versions of the extensions?

…

On Thu, 19 Jan 2017, 18:38 Clayton Coleman, ***@***.***> wrote: Yes this would help us - we currently perform a Go AST traversal of the generated types to remove it and this would remove the need for us having to do that. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#250 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvsLSbWNU0PO2TmL-1rgnbEwpr_DS3Tks5rT59wgaJpZM4LfMht> .

* `drop_type_declaration` becomes `typedecl` * `enum_drop_type_declaration` becomes `enumdecl` Default values are now true and for removing the declaration the value needs to be `false`. Options `typedecl_all` and `enumdecl_all` are added.

Serabe · 2017-01-20T11:16:25Z

@awalterschulze I readded the comment, renamed the options and added the file-scoped versions with tests. Is this ok?

awalterschulze · 2017-01-20T12:58:22Z

I am pretty sure it will be, but I'll check when I get home :)

…

On Fri, 20 Jan 2017 at 12:16 Sergio Arbeo ***@***.***> wrote: @awalterschulze <https://github.com/awalterschulze> I readded the comment, renamed the options and added the file-scoped versions with tests. Is this ok? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#250 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvsLaKmWdvEW3AC22ioKGKRS512KssZks5rUJeKgaJpZM4LfMht> .

awalterschulze · 2017-01-20T15:05:00Z

extensions.md

 <tr><td><a href="http://godoc.org/github.com/gogo/protobuf/plugin/marshalto"> unsafe_marshaler</a> </td><td> Message </td><td> bool </td><td> if true, a Marshal and MarshalTo method is generated for the specific message. The generated code uses the unsafe package. </td><td> false</td></tr>
 <tr><td><a href="http://godoc.org/github.com/gogo/protobuf/plugin/unmarshal">unsafe_unmarshaler</a></td><td> Message </td><td> bool </td><td> if true, an Unmarshal method is generated for the specific message. The generated code uses the unsafe package. </td><td> false</td></tr>
 <tr><td><a href="http://godoc.org/github.com/gogo/protobuf/plugin/marshalto">stable_marshaler</a></td><td> Message </td><td> bool </td><td> if true, a Marshal and MarshalTo method is generated for the specific message, but unlike marshaler the output is guaranteed to be deterministic, at the sacrifice of some speed</td><td> false </td></tr>
+<tr><td>typedecl (beta)</td><td> Message </td><td> bool </td><td> if false, type declaration of the message are excluded from output. </td><td> true </td></tr>


if false, type declaration of the message is excluded from the generated output

awalterschulze · 2017-01-20T15:05:12Z

extensions.md

 <tr><td><a href="http://godoc.org/github.com/gogo/protobuf/gogoproto"> castkey </a> (beta) </td><td> Field </td><td> string </td><td> Changes the generated fieldtype for a map key.  All generated code assumes that this type is castable to the protocol buffer field type.  Only supported on maps. </td><td> goprotobuf field type </td></tr>
 <tr><td><a href="http://godoc.org/github.com/gogo/protobuf/gogoproto"> castvalue </a> (beta) </td><td> Field </td><td> string </td><td> Changes the generated fieldtype for a map value.  All generated code assumes that this type is castable to the protocol buffer field type.  Only supported on maps. </td><td> goprotobuf field type </td></tr>
 <tr><td>enum_customname  (beta)</td><td> Enum </td><td> string </td><td>Sets the type name of an enum. If goproto_enum_prefix is enabled, this value will be used as a prefix when generating enum values.</td><td>goprotobuf enum type name. Helps with golint issues.</td></tr>
+<tr><td>enumdecl (beta)</td><td> Enum </td><td> bool </td><td> if false, type declaration of the enum are excluded from output. </td><td> true </td></tr>


if false, type declaration of the enum is excluded from the generated output

awalterschulze · 2017-01-20T15:10:08Z

test/typedecl_all/typedeclall.proto

+option (gogoproto.testgen_all) = true;
+option (gogoproto.populate_all) = true;
+option (gogoproto.benchgen_all) = true;
+option (gogoproto.unmarshaler_all) = true;


Interestingly this probably won't work as well when the marshaler and unmarshaler are not generated. Perhaps we should make a note of this in the docs, that it is not supported without code generation

Serabe · 2017-01-20T15:33:20Z

@awalterschulze done! Thank you!

awalterschulze · 2017-01-20T15:49:56Z

Oh last thing. Do you want to add yourself to the CONTRIBUTORS or AUTHORS file as appropriate?

Serabe · 2017-01-20T16:06:28Z

Done, thank you!

awalterschulze · 2017-01-20T16:23:06Z

Thanks for one of the best features in a long time. This is really great work :)

Serabe · 2017-01-20T18:59:52Z

Thanks!

…

On 20 January 2017 at 17:23:07, Walter Schulze ***@***.***) wrote: Thanks for one of the best features in a long time. This is really great work :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#250 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACNfYNrSUaZFF1uyuI4BnbY4xM1A3nKks5rUN9rgaJpZM4LfMht> .

awalterschulze reviewed Jan 17, 2017

View reviewed changes

Serabe added 2 commits January 18, 2017 09:21

[FEATURE] Add options for dropping type declarations

fb1c0f2

Add two options, one for messages and one for enums, to drop type declarations. In messages, it also drops getters and setters. In enums, it also drops the generated constants.

[GENERATED] Add options for dropping the type

fed5238

Serabe force-pushed the feature/drop_type_declaration branch from 20ebf70 to fed5238 Compare January 18, 2017 08:26

Fix drop type decl tests Makefile

9746608

Fix protoc calls

bb245dc

awalterschulze reviewed Jan 18, 2017

View reviewed changes

Rename test proto files and fix makefiles.

8cb7432

Make drop type declaration and go getters options independent

e52a6c3

This commit extract a few variable initializations that was being done at the same time of generating the type for a given message. This allow for the Go Getters option to be independent from the drop type declaration one

awalterschulze reviewed Jan 19, 2017

View reviewed changes

Readd comment

0120fe7

Serabe added 2 commits January 20, 2017 12:10

[GENERATED] Side effects of previous commit

19aab7d

Serabe mentioned this pull request Jan 20, 2017

Use gogo/protobuf again src-d/proteus#45

Merged

awalterschulze reviewed Jan 20, 2017

View reviewed changes

Improve documentation

2594db4

Add me to contributors

8657561

awalterschulze merged commit 265e960 into gogo:master Jan 20, 2017

Serabe deleted the feature/drop_type_declaration branch January 23, 2017 08:24

Drop types declaration option #250

Drop types declaration option #250

Uh oh!

Conversation

Serabe commented Jan 10, 2017

Uh oh!

awalterschulze commented Jan 10, 2017

Uh oh!

awalterschulze commented Jan 10, 2017

Uh oh!

awalterschulze commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Serabe commented Jan 18, 2017

Uh oh!

Serabe commented Jan 18, 2017

Uh oh!

awalterschulze commented Jan 18, 2017

Uh oh!

awalterschulze commented Jan 18, 2017

Uh oh!

Serabe commented Jan 18, 2017

Uh oh!

Serabe commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awalterschulze commented Jan 18, 2017

Uh oh!

Serabe commented Jan 18, 2017

Uh oh!

awalterschulze commented Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Serabe commented Jan 18, 2017

Uh oh!

Serabe commented Jan 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awalterschulze commented Jan 19, 2017

Uh oh!

smarterclayton commented Jan 19, 2017

Uh oh!

awalterschulze commented Jan 19, 2017 via email

Uh oh!

Serabe commented Jan 20, 2017

Uh oh!

awalterschulze commented Jan 20, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

awalterschulze commented Jan 18, 2017 •

edited

Loading