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

Support for gogoproto #52

Closed
kyessenov opened this issue Jan 17, 2018 · 7 comments
Closed

Support for gogoproto #52

kyessenov opened this issue Jan 17, 2018 · 7 comments
Labels
Enhancement Extend or improve functionality Go Go language support Wont Fix Out of scope of this project

Comments

@kyessenov
Copy link
Contributor

kyessenov commented Jan 17, 2018

It seems that this tool only supports the standard goproto code generation output. I have noticed the following deviations for gogo:

  • disregard of go_package annotation that allows multiple proto files to be combined into one package, to avoid circular dependencies between packages
  • no support for gogoproto code generation optimization annotations:
    • gogoproto.nullable changes a pointer to an embedded struct for fields that are not nullable by construction
    • gogoproto.stdduration changes a duration from ptypes.Duration to time.Duration

What needs to be done to extend PGV with gogo support?

@rodaine
Copy link
Member

rodaine commented Jan 17, 2018

disregard of go_package annotation that allows multiple proto files to be combined into one package, to avoid circular dependencies between packages

This is probably an oversight in the template, not using the correct package value. I can look into this.

no support for gogoproto code generation optimization annotations:

I haven't worked with gogoproto, so I don't have specifics here, but I'd say we should treat this as a different language target since it deviates in some fundamental ways from the official protoc-gen-go output. A lot could be reused between the two, though.

@freman
Copy link

freman commented Oct 15, 2018

Tis a good start to be sure to be sure, but I have run into an issue with (gogoproto.customname)

./dhcp.pb.validate.go:135:11: m.GetTtl undefined (type *InterfaceConfiguration has no field or method GetTtl, but does have GetTTL)

@kyessenov
Copy link
Contributor Author

customname is not supported last time I checked.
It should not be hard to add extra handling for customname. Seems like you might need to override accessor here https://github.com/lyft/protoc-gen-validate/blob/master/templates/goshared/register.go#L88.

@rvolosatovs
Copy link
Contributor

rvolosatovs commented Feb 26, 2019

I added GoGo support to protoc-gen-star directly: lyft/protoc-gen-star#51
Using that functionality I added full support for customname/customtype/casttype/embeded: TheThingsIndustries@0065aca
And validation of non-nullable messages: TheThingsIndustries@228ba7a

If approach in lyft/protoc-gen-star#51 seems viable and it gets merged, I can open PR to this repo for improving the GoGo support.

FTR, in our fork (https://github.com/TheThingsIndustries/protoc-gen-validate) we added support for fielpaths in validators, so the caller can specify a subset of fields to validate. If no paths are specified - all fields are validated, same as the original plugin.

@coolyrat
Copy link

coolyrat commented Jun 19, 2019

Is there any plan supporting gogoproto.embed? Embed reduce a lot of duplicate codes. Right now I have to skip the embed message.

message AdPrimaryKey {
    string media = 1;
    string name = 2;
}

message Ad {
    AdPrimaryKey pk = 1 [(gogoproto.embed) = true, (validate.rules).message.skip = true];
    AdType type = 2 [(validate.rules).enum.not_in = 0];

    enum AdType {
        NONE = 0;
        OTHERS = 1;
        VIDEO = 2;
    }
}

message UpdateAdRequest {
    AdPrimaryKey pk = 1 [(gogoproto.embed) = true, (validate.rules).message.skip = true];
    Ad ad = 2;
}

@monotone
Copy link

care about this too.

@rvolosatovs
Copy link
Contributor

care about this too.

Given that the project is unmaintained for a while gogo/protobuf#691 I would not expect this to happen

@elliotmjackson elliotmjackson added the Wont Fix Out of scope of this project label Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Extend or improve functionality Go Go language support Wont Fix Out of scope of this project
Projects
None yet
Development

No branches or pull requests

7 participants