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

Idiomatic/pointer codegen for well known types (Int64Value, etc) #472

Closed
stephen opened this issue Aug 28, 2018 · 5 comments · Fixed by #489
Closed

Idiomatic/pointer codegen for well known types (Int64Value, etc) #472

stephen opened this issue Aug 28, 2018 · 5 comments · Fixed by #489

Comments

@stephen
Copy link

stephen commented Aug 28, 2018

Hi there,

In the spirit of more idiomatic go generation, it'd be really nice if well known wrapper types (Int64Value, StringValue, etc.) could emit pointer versions of themselves.

I think this would help a lot for issues like:

A proposed change might look like:

message Thing {
  google.protobuf.Int64Value value = 1 [(gogoproto.wktpointer) = true];
}

would emit:

type Thing struct {
  Value *int64
}

and in the Unmarshal / Marshal emit, we could convert things back to the WKT discreetly.

cc/ @awalterschulze

@stephen stephen changed the title Idiomatic codegen for well known types (Int64Value, etc) Idiomatic/pointer codegen for well known types (Int64Value, etc) Aug 28, 2018
@awalterschulze
Copy link
Member

@donaldgraham and @jmarais are the new maintainers for this repo.

But for context:

I think the same issue has been opened against golang/protobuf golang/protobuf#414

This proposal is on hold until reflection work is finished (see golang/protobuf#364) as generating native Go types will need to inter-operate with proto reflection in a reasonable way. - Joe Tsai

Personally, I think it is a really good proposal, but building a feature, which golang/protobuf might build in future and then having to juggle support for the gogo version of the feature versus the golang version of the feature makes things hard.

@stephen
Copy link
Author

stephen commented Aug 29, 2018

Thanks for the note, I agree that it's the same issue. I'm glad this is being considered, though I'm a sad that it's blocked on a couple of fairly large looking projects.

@donaldgraham, @jmarais, do you have a sense for what you'd like to see happen here? For us, I think it's impractical for us to try to contribute an implementation if this package would prefer waiting on golang/protobuf#414 to shake out first.

I'd love to see more idiomatic codegen and this feels like a good approach. I worry that that set of work in golang/protobuf is fairly large, so it may take a long while before golang/protobuf#364, then golang/protobuf#414 sees the light of day. It appears to me that the api-v2 branch is a total rewrite of the package, judging from https://github.com/golang/protobuf/tree/api-v2, so I'm not sure how that plays into gogoproto's plans.

@donaldgraham
Copy link
Contributor

I don't have a major problem building the feature now and then refactoring it if it gets built into golang/protobuf. @jmarais, do you think we can plan to do this quite soon?

virtuald added a commit to virtuald/protobuf that referenced this issue Sep 21, 2018
virtuald added a commit to virtuald/protobuf that referenced this issue Sep 21, 2018
@virtuald
Copy link
Contributor

I would love for #472 to be fixed. It seemed like it wouldn't be so bad so I sat down for a bit tonight and did some repetitive codegen myself .. but now I'm a bit stuck trying to get the stdtypes test to work and I won't have time for at least a week or so to get back to this.

I've got a branch wkt-codegen that has a lot of the repetitive stuff already taken care of if someone else wants to take this to the finish line. 🏁

virtuald added a commit to virtuald/protobuf that referenced this issue Sep 21, 2018
virtuald added a commit to virtuald/protobuf that referenced this issue Sep 21, 2018
virtuald added a commit to virtuald/protobuf that referenced this issue Sep 21, 2018
@virtuald
Copy link
Contributor

Looks like I found time to fix it up today. See #489 for the changes.

virtuald added a commit to virtuald/protobuf that referenced this issue Sep 21, 2018
virtuald added a commit to virtuald/protobuf that referenced this issue Sep 21, 2018
virtuald added a commit to virtuald/protobuf that referenced this issue Sep 21, 2018
virtuald added a commit to virtuald/protobuf that referenced this issue Sep 21, 2018
virtuald added a commit to virtuald/protobuf that referenced this issue Sep 21, 2018
virtuald added a commit to virtuald/protobuf that referenced this issue Sep 24, 2018
virtuald added a commit to virtuald/protobuf that referenced this issue Sep 24, 2018
virtuald added a commit to virtuald/protobuf that referenced this issue Sep 24, 2018
donaldgraham pushed a commit that referenced this issue Oct 26, 2018
* Codegen for well-known types

- Fixes #472

* Regenerate protobufs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants