Mention Protobuf Style Guides on the v7 protocol

[marioizquierdo]

Issue #244

Mention why Twirp implementations should respect CamelCased names, and not worry too much about other ways to map other identifiers into their language naming conventions.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[thesilentg]

Is this implying that Twirp implementations may not fully support names that do not follow these conventions? That is to say, if you skip these conventions you're doing so "at your own risk" of compatibility problems. If this is the case, I think a little stronger warning may be needed here.

[marioizquierdo]

We've been back and forth at length over this on #244

The issue is that, this is technically not a protocol problem, but the Go implementation does not respect the protocol, and looking closer it is not possible to entirely avoid naming collisions. This problem can be avoided by users if they respect the Protobuf Style Guide. And even if they don't it is usually not a problem because it is rare that someone uses potentially conflicting names like MyMethod and myMethod.

We started including the recommendation for users on the docs: https://github.com/twitchtv/twirp/blob/master/docs/routing.md#naming-style.

The message here in the protocol spec is for developers working on a new Twirp implementation, in case they are wondering how to ensure full compatibility with the spec and other language implementations. Which is what happened to @ofpiyush, that started this conversation in detail.

Does it make more sense? Maybe I could use different words to make that a little more clear?

[thesilentg]

If this is for developers working on a new Twirp implementation, I'd say give them the ability to treat names which do not follow the style guide in any way they desire (C-style Undefined Behavior). Something like "Twirp implementations MUST support names that follow the Protobuf Style Guide, and names which do not follow the Protobuf Style Guide MAY be modified by the implementation."

We should then update the Twirp user documentation to say that cross-language compatibility guarantees are only made if a user follows the Protobuf Style Guide for naming.

[ofpiyush]

The issue I (and other language implementations) ran into was with the wire protocol, not service and method names of generated code. Specifically, which URL should one serve and send requests to.

Service/ Method naming convention is only required for languages where case is meaningful. Go is the only language with this feature as far as I know.

In all other languages, one can just use the literal name for service names and methods in whatever format the protobuf file has, without any issues.

[marioizquierdo]

I'll add a few more notes in the routing documentation. The protocol can be left without details about the Go implementation, the Service and Method parts of the routes should match the definitions in the proto files exactly, and the Go implementation will soon be able to do this thanks to #257 and #277

[ofpiyush]

For go specifically, I ran some tests on what protoc-gen-go does with messages and fields. Both github.com/golang/protobuf and google.golang.org/protobuf generate with the same naming convention.

The only case where proto3 errors out is if field names are same except for case. That breaks their JSON naming convention. proto2 adds an _ at the end for the 2nd time it encounters the same name.

Here is a table of what protoc-gen-go does silently. Doing these together will break at compile time.

proto message name	go struct name
myMessage	MyMessage
MyMessage	MyMessage
my_message	MyMessage
My_message	MyMessage
my_Message	My_Message
My_Message	My_Message

If you're looking for a way out of this for Service and Method names. I'd say talking to them might be the best course of action as the convention they'll follow to resolve it would be the one this project should follow as well.

To get things started, I've opened an issue over at golang/protobuf#1206

[ofpiyush]

Update: so far the attitude on that thread has been "won't fix" and "go talk to protoc".
I don't think they will come up with any solution.

I agree that multiple source names resulting in same conflicting output is not a big enough use case to truly fix.

To move forward, twirp can pick any solution that it likes.

A potential solution I found was to fail at build time by checking for package level errors.
https://github.com/golang/go/blob/23573d0ea225d4b93ccd2b946b1de121c3a6cee5/src/go/ast/resolve.go#L74

conf := &types.Config{Importer: importer.Default()}
_, err = conf.Check("", fset, []*ast.File{fileAST}, nil)

Full disclosure, protoc-gen-go doesn't look too keen on adding it.