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?

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

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!

awalterschulze commented Jan 18, 2017 •

edited

Loading