enum_customname might be enough.

We could then have a vanity binary that adds customnames to each enum value that does not satisfy the gostyle?

Tests would be good to make sure it plays well with all the other plugins.

Perhaps, but I thought enum_customname was a little verbose under use when the goal was to have style compliant constant.

We could then have a vanity binary that adds customnames to each enum value that does not satisfy the gostyle?

Do you have an example of a "vanity binary"? Does this require an extra build step or can it be activated by an extension declaration?

I'll make sure to get testing and documentation in when we have the right feature set.

@awalterschulze I took a quick peak at the vanity plugins and they seem to oriented towards additions, rather than modifications of actual fields. I suspect I am missing something, but is there a way to preprocess the descriptor and add the custom names when gostyle is enabled? That seems like the right approach.

Interesting hmmm.

vanity binaries can make almost any modification to the descriptor (parsed protocol buffer definition).
I just coded some helper functions. All the helper functions are additions, since I haven't encountered any other usecases, ... yet.

If we can build a vanity binary that adds enum_customname extensions to enum values then we can probably make one that first checks whether enum_gostyle is enabled for that enum.

It would be the first time gogoprotobuf does something like that though. Typically vanity binaries are used to programatically add (and not overwrite) some extensions and not first check another extension before deciding to do the modifications.
I don't know if it is a little overkill at this point.

Why not create enum_customname and the vanity binary, with tests, since we'll need that code anyway and then we can decide whether we think we also need enum_gostyle?

@awalterschulze I am confused what you mean by "vanity binary". Are you talking about something like protoc-gen-gogoslick? Would it be something like protoc-gen-gogostyle? My understanding was that there were just binaries with common sets of options that could be used together without requiring changes in the source protobuf file.

Also, are you saying that I should do this instead of what is presented in this diff or in addition to?

It would be the first time gogoprotobuf does something like that though. Typically vanity binaries are used to programatically add (and not overwrite) some extensions and not first check another extension before deciding to do the modifications.

It seems like this is already done with the goproto_enum_prefix. If goproto_enum_prefix is disabled for the field, it falls back on the setting of goproto_enum_prefix_all. It's possible I'm misunderstanding what you're saying, but the proprosed goproto_enum_gostyle seems very similar. It is not actually creating a goproto_enum_customname entry on the descriptor. It just allows favors goproto_enum_customname over goproto_enum_gostyle.

Why wouldn't these just be regular options like the others? This is what I have working now with this PR:

enum Value {
    option (gogoproto.enum_gostyle) = true;
    FOO = 0;
    BAR = 1;
    BAZ = 2;
}

This generates the following:

type Value int32

const (
    ValueFoo MessageType = 0
    ValueBar MessageType = 1
    ValueBaz MessageType = 2
)

If I'm understanding this correctly, this should just work with the regular protoc-gen-gogo binary. I'd rather not have a different build step for files that use these extensions, if possible. It seems to make more sense just to have support for it in the generator itself.

Sorry for the delay, today was quite busy. I'll get to this asap.

@awalterschulze No worries. I just want to make sure I understand everything before proceeding. Any pointers on what to do would be helpful.

Yes vanity binaries are exactly like protoc-gen-gogoslick, etc.
These binaries apply options in some way so that you can write less extensions.
So my idea is that a vanity binary, for example protoc-gen-gogostyle, can add enum_custom_name to all enums that need gostyle improvements.
That way we avoid adding another extension gostyle and gostyle_all in the mix.

But if you only want to enable it for certain files only, you can't just use a vanity binary. Then we could rather have protoc-gen-gogo use vanity functions to check for gostyle and gostyle_all and apply the customname extension just like the vanity binary would have.

gostyle and gostyle_all would then not overwrite any customnames already defined.

Then to the user they can be normal extensions, just like the others, but the implementations would only have to worry about customname. Except obviously for the part where we modify the descriptor and add the customname extensions according to the rules of gostyle and gostyle_all.

How does that sound?

On another note, why only gostyle for enums?

But if you only want to enable it for certain files only, you can't just use a vanity binary. Then we could rather have protoc-gen-gogo use vanity functions to check for gostyle and gostyle_all and apply the customname extension just like the vanity binary would have.

Interesting. Would this require adding a few vanity passes in the protoc-gen-gogo binary? This sounds reasonable.

gostyle and gostyle_all would then not overwrite any customnames already defined.

Then to the user they can be normal extensions, just like the others, but the implementations would only have to worry about customname. Except obviously for the part where we modify the descriptor and add the customname extensions according to the rules of gostyle and gostyle_all.

This is somewhat the intent, although this logic is pushed down to the final generation stage, rather than a modification of the descriptor. I'll see what I can do.

On another note, why only gostyle for enums?

It is restricted to enums because I thought they were the most egregiously ugly and the easiest to change. The other changes might take some more complex logic (ie Id to ID, et. al). I see no reason to limit the scope of this.

I'll revisit this in a few days and update this PR.

Basically I want the generation code to only support customname and a pregeneration (vanity) step to modify the descriptor to support gostyle and gostyle_all.

A few passes sounds weird, but I think we are on the same page. This pregeneration (vanity) step would be built into gogoprotobuf (for the first time).

Ok lets start with enums and if that is working perfectly we quickly list the extra code that would be required for a possible gostyle_all.

I know +1's are not in style anymore, but +1 overall to this PR, specifically enum_customname. I end up doing this https://github.com/libopenstorage/openstorage/blob/b49fab4850a072b05a0bc904ef8c1ad65f27c713/api/api.go#L61 all the time since I always prefix enum names with their type due to the c++ scoping rules.

@peter-edge I'm working with this branch right now and it works great. I still need to go back and do some of the refactoring to integrate these with the vanity binaries. I may make a few additions to give us nice still all around. Working with these as concrete types has proven performant and simple (probably preaching to choir).

yep i am excited for this to be ready :)

@awalterschulze I spent some time experimenting with the vanity binary approach. I mimicked the cockroachdb setup to understand it better and used it on a toy project with great success.

Here is my plan:

• The enum_customname extension is going to be broken out into a separate PR. That is a very straightforward change and I'd like to get it in. (gogoproto: enum and enum value custom name extension #148)
• Submit a larger arch PR that attempts to provide the global gostyle extension
  • Requires the generator interface to make a pass over the descriptors before generating the files.
  • We must come up with transform that will output code that passes the golint (see https://github.com/golang/lint/blob/master/lint.go#L510 for information on this transform)
  • Implement modifications to customname entries in the pre-generation descriptor pass.

This is larger than I'd like to take on but I believe the result will be more than worth it. At this time, I'm going to submit the enum_customname PR and if you agree to the above, I'll open an issue with the plan above and we can work from there.

I think the plan sounds good. Separating it out into steps is fine as long as each of those steps are well tested.

Just be warned: I know you are not alone in wanting golintable generated files, but you are going down a path that, for reasons I don't fully understand yet, golang/protobuf is totally unwilling to go down.
If I fully understood why I wouldn't have warned you, but since I don't, there is a chance that we might find something on our path that is harder than we think.
I am optimistic, but also little concerned :)

@awalterschulze Thanks for the fair warning. I suspect there will be odd issues about deterministically generating variable names. This is partially why I've split the plan out this way. If we don't get full gostyle, we still get a descriptor modification pass in the generator that could be useful elsewhere.

I'll setup the issue to track all this.

cool :)

Closing this in favor of plan in #149.