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

Add descriptor package #131

Closed
wants to merge 1 commit into from
Closed

Add descriptor package #131

wants to merge 1 commit into from

Conversation

myitcv
Copy link
Member

@myitcv myitcv commented Feb 15, 2016

Per 0dfe8f3 et al

@@ -58,6 +59,7 @@ for up in "${!filename_map[@]}"; do
cat $tmpdir/$UPSTREAM_SUBDIR/$up |
# Adjust proto package.
# TODO(dsymonds): Upstream the go_package option instead.
grep -v '^\s*option\s\+go_package' |
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hack can similarly be removed once we consistently have go_package set in the upstream .protof files

@dsymonds
Copy link
Contributor

Descriptor isn't really a "well-known type" like the others are.

@myitcv
Copy link
Member Author

myitcv commented Feb 16, 2016

It's required when defining custom options:

https://developers.google.com/protocol-buffers/docs/proto#customoptions

When defining a custom option the generated code references either
FileOptions or FieldOptions (both defined in descriptor.proto)
On 16 Feb 2016 21:49, "David Symonds" notifications@github.com wrote:

Descriptor isn't really a "well-known type" like the others are.


Reply to this email directly or view it on GitHub
#131 (comment).

@dsymonds
Copy link
Contributor

That doesn't really address what I said.

@myitcv
Copy link
Member Author

myitcv commented Feb 17, 2016

@dsymonds sorry, I didn't mean to suggest that the descriptor package contains "well-known" types like Duration et al (especially given the documentation describes custom options as an advanced feature)

Rather that we could reuse the mechanism you have created for generating the well-known types for the descriptor package.

Unless you had a different approach in mind for the descriptor package, or more generally custom options?

@bufdev
Copy link

bufdev commented Mar 8, 2016

Most people use https://github.com/golang/protobuf/tree/master/protoc-gen-go/descriptor, although it would be nice to add the descriptor.proto file there as well.

@Globegitter
Copy link

Globegitter commented Apr 28, 2016

I tested this PR out on a fork (https://github.com/Globegitter/protobuf) and to get it to work I had to add a descriptor.go file into the ptypes folder: https://github.com/Globegitter/protobuf/blob/master/ptypes/descriptor.go

Otherwise I would get proto_name.pb.go:34: can't find import: "github.com/globegitter/protobuf/ptypes/descriptor"

Other than that it would be great to get this in to be on par with some of the other protobuf libraries where you don't need to specially add the descriptor proto and it just works.

@yohcop
Copy link

yohcop commented Oct 27, 2016

should the other descriptor.pb.go in protoc-gen-go/descriptor be removed? (this pull request is very old now, so at that time maybe the one in protoc-gen-go/descriptor didn't exist... but still, something to remember before merging)

@dsnet
Copy link
Member

dsnet commented Mar 8, 2018

This already exists in protoc-gen-go/descriptor.

@dsnet dsnet closed this Mar 8, 2018
@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants