gostyle: provide option to generate golint compatible protobufs

[stevvooe]

Many users of protobuf would prefer to directly use generated structures within their application. The main benefit of using the generated types directly is reduced copying, allocation and bugs when marshaling and unmarshaling types. The prevailing issue is that generated protobuf code does not pass golint. Such types also don't work well with godoc documentation.

We experimented with this concept applied to enums in #133. The results were encouraging but others felt that we should provide this option for other types, at the file level.

There are several steps that need to happen to provide this feature.

Here is current plan:

• [x] Implement enum_customname extension to allow one to provide custom names for enum values. This will address #94. gostyle transforms will leverage this across the board. (#148)
• [ ] Implement message_customname to allow option to modify message name, either through plugin preprocessor or via protobuf file.
• [ ] Address pain points:
  • [x] Generated oneof can be awkward to use. Investigate this further and come up with suggestions (see #168). Not realistic. Maybe, we generate helpers, but not necessary on this issue.
  • [x] time.Time and time.Duration are painful to use. This may be solved by golang/protobuf in future but we'd like to generate code that targets the WKTs but uses Go typed fields. There may be problem with ranging here, but most applications don't need the larger ranges. (Need #155)
• [ ] Implement gostyle extension
  • [ ] Add a "transform" step to allow generators to make a pass over the descriptor to apply extensions before generation.
  • [ ] Define variable name transforms that are compatible with 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.

It has been pointed out that there may be unforeseen complexities in this endeavor. If the plan changes or we need more steps, I'll try to update this issue accordingly.

If anyone wants to help in this effort, please coordinate here.

[awalterschulze]

I have also only recently seen the problems with godoc at the package level. I don't really want the godoc reader to know that this is protocol buffer package, only that there exists protocol buffer messages within the package. I will investigate this a little soon, hopefully next week.

customnames might also be required for messages. customnames for fields is luckily already complete.

[awalterschulze]

Ok so I would like to merge #148 , but we should discuss the choice of whether to 1) have gostyle set enum_prefix to false and have enum_customname be used to change the field name to something appropriate, whether this included the prefix is also up for discussion, 2) have enum_customname be a conflicting extension with enum_prefix (you can't have both at the same time) and give enum_customname full control over the whole field name. This way we could even implement enum_prefix with enum_customname and some vanity magic.

Currently #148 implements option 1. Is this what we want in the long term? I liked option 2 the most, but I think its extra work for very little gain. And if we choose 1 we could still leave it up to the user to decide whether the person wants a prefix or not. So in the end I like option 1 the most, since the user gains extra flexibility. If you agree. I'll merge #148

[stevvooe]

I agree with your assessment.

The issue with option 2 is that we effectively break compatibility. We would have to change enum_prefix to drop the _, which is undesirable. I went down this path slightly and there wasn't a way to preserve the current behavior and have the gostyle-style output be auto-calculated using prefix and customname.

Ultimately, I think this works, since a user configuring gostyle would likely subscribe to the concept of have enum_prefix ignored in favor or Go style guide compatible enums, which are prefixed without the underscore.

Merge away!

[tamird]

@stevvooe what's the state of this? I'm interested in seeing this happen.

[stevvooe]

@tamird I'm slowly working through this. Right now, our usage of enum_customname + enum_prefix=false with enumvalue_customname is fairly common. Thus far, the behavior has been solid.

I think before the work above proceeds, we'll need a solid message_customname option.

Here is a short list of other places we are hurting:

1. oneof types are awful to use.
2. Support for mapping time.Time and time.Duration directly to protobuf Timestamp and Duration. This is a nightmare right now and it doesn't need to be.

As you can see, this may end up being a slow process but any help here would be appreciated (that is actually why I broke it down). I'll update the list in the description.

[awalterschulze]

Agreed message_customname is needed if you want to fully go lint.

I don't think proto3 support is done until well known types like time.Time and time.Duration are supported by gogoprotobuf. So I'll probably do that work. I am just waiting on goprotobuf to properly support WKT.

We have not really started to use oneof that much. Can you think of a better api?

[dennwc]

@stevvooe @awalterschulze Totally agreed regarding oneof. Maybe we should open a separate issue to discuss it further?

[awalterschulze]

Yes if you have any suggestions, please feel free to open another issue regarding oneof.

[stevvooe]

@awalterschulze @dennwc I agree that we should break these chunks off into other issues. Hopefully we can track concerns here and address them individually.

I started some explorations in the other issue.

[stevvooe]

@awalterschulze I made a few updates to the description.

[awalterschulze]

Perfect :)

[awalterschulze]

Are we planning to continue working on this case?