Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APIv2: consistent package naming #848

Closed
neild opened this issue May 8, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@neild
Copy link
Contributor

commented May 8, 2019

The new repo contains a number of public packages (and a larger number of internal ones, but those aren't interesting):

encoding/jsonpb
encoding/textpb
proto
proto/dynamicpb
protogen
reflect/protoadapt
reflect/protodesc
reflect/protoreflect
reflect/protoregistry
testing/prototest
types/descriptor
types/known
types/plugin

Leaving aside types/..., which is something of a special case, we've got two different naming conventions going on here: protofoo and foopb. We should converge on a single convention, or at least understand why we're being inconsistent.

Most imports of the protofoo packages in the new repo use local aliases to make the name shorter: pref for protoreflect and so forth. Perhaps that points at the protofoo name being too verbose?

On the other hand, the foopb names are confusing to my eyes because Google has a local style convention of using a foopb alias for imports of generated proto packages. That's just a local convention, though, which I don't think is prevalent in the wider world.

Some possible options:

  • protofoo: encoding/protojson, encoding/prototext, etc.
  • foopb: reflect/reflectpb, reflect/registrypb, etc.
  • pbfoo: encoding/pbjson, reflect/pbreflect, etc.
  • pfoo: encoding/pjson, reflect/preflect, etc.
  • ¯_(ツ)_/¯: encoding/jsonpb, reflect/protoreflect, testing/chicken.

I don't really have a preference, but we're getting to the point where this needs to be sorted out.

@cybrcodr

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

I agree with coming up with better consistency. I do also think that some of the names are a bit too verbose.

Here are some other suggestions:

encoding/jsonpb        -> encoding/json
encoding/textpb        -> encoding/text
proto/dynamicpb        -> proto/dynamic or dynamic
reflect/protoreflect   -> reflect
reflect/protodesc      -> reflect/desc
reflect/protoregistry  -> reflect/registry

Get rid of the proto prefix and pb suffix in package names. When I wrote jsonpb and textpb packages, I went with those names because (1) simply following v1's jsonpb name, and (2) was avoiding similar package name as the standard lib encoding/json. But neither of these reasons are any good.

For reflect/protoreflect, if we get rid of proto prefix, it becomes a stutter reflect/reflect, so may as well move that to simply reflect. But perhaps by the same token, since the import path already contains protobuf name, then anything under proto path should be lifted one level up? Although I somehow am not as bothered by having the proto path.

@neild

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

I'd like to avoid conflicting with existing popular package names, so I don't like reflect or json; it's too confusing to use the same name as the stdlib reflect or encoding/json packages.

@dsnet

This comment has been minimized.

Copy link
Member

commented May 8, 2019

My vote is to have a consistent proto prefix (e.g., encoding/proto{json,text}). But I'd be okay with a consistent pb prefix or suffix.

One argument against the pb suffix is that it is commonly used to identify packages that represent generated protobuf messages. This would result in a package named descpb (for the current reflect/protodesc) and descriptorpb (for the current types/descriptor).

Most imports of the protofoo packages in the new repo use local aliases to make the name shorter: pref for protoreflect and so forth. Perhaps that points at the protofoo name being too verbose?

I'm most tempted to rename the protoreflect package in situations with a type switch and many constants in the case. I'd like to see that pattern be simplified to needing one expression per case. Speaking for myself, I've unnecessarily renamed packages primarily because I did it once for protoreflect and it set the precedence to keep renaming other packages.

it's too confusing to use the same name as the stdlib reflect or encoding/json packages

It's confusing for humans, and it's confusing for a tool like goimports. When it sees json.Marshal, it has no idea whether the author's intent is to use encoding/json or google.golang.org/protobuf/encoding/json.

Other miscellaneous thoughts:

  • I'd prefer if protogen were under generator/protogen or compiler/protogen. It's unlikely that we'd add a Go implementation of a .proto parser (#841). However, if we do end up implementing protoparse, I'd like for it to have an more clear place to live.
  • I feel like dynamicpb should be under types. At least in my mind, everything under types provides Go types that implement protobuf messages, which is what dynamicpb is. However, if types is only for generated packages, then I can see why it doesn't belong there.
@puellanivis

This comment has been minimized.

Copy link

commented May 8, 2019

When it sees json.Marshal, it has no idea whether the author's intent is to use encoding/json or google.golang.org/protobuf/encoding/json.

I add here: text/template vs html/template when people rely too heavily on goimports to add their imports for them, the two are identical. Then it takes awhile to even notice that something is wrong, really it needs an unmatched < on a line… then they wonder what is wrong.

@neild

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

We definitely need to make sure things work properly with goimports.

I'd prefer if protogen were under generator/protogen or compiler/protogen.

SGTM. The main protobuf distribution uses the word "compiler", so I'd go with that.

I feel like dynamicpb should be under types.

I think we should keep the generated packages separate, but otherwise I don't have a strong opinion.

@cybrcodr

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

it's too confusing to use the same name as the stdlib reflect or encoding/json packages.

Fair enough. I just think that tools should be better at these, but agree that it may confuse humans too.

My vote on using proto prefix then except for the generated types and dynamicpb. I prefer to have the pb suffix as well for the generated types. And do feel like dynamicpb should be under types as well.

@neild

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Renamed jsonpb, textpb, and protogen:

google.golang.org/protobuf/encoding/protojson
google.golang.org/protobuf/encoding/prototext
google.golang.org/protobuf/compiler/protogen

https://go-review.googlesource.com/c/protobuf/+/177178

@neild neild closed this May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.