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.

Sign up for GitHub

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

Jump to bottom

*: add proto3 optional support #431

Closed
1 of 2 tasks
glerchundi opened this issue Feb 21, 2021 · 19 comments · Fixed by #537
Closed
1 of 2 tasks

*: add proto3 optional support #431

glerchundi opened this issue Feb 21, 2021 · 19 comments · Fixed by #537
Labels
Enhancement Extend or improve functionality

Comments

@glerchundi
Copy link
Contributor

glerchundi commented Feb 21, 2021 •
edited
Loading

I think it would be good to start thinking on how to add support to the proto3 optional keyword.

This feature request could be addressed in two different steps:

WDYT?
@xmlking xmlking mentioned this issue Mar 2, 2021
Closed
@ivanovishado
Copy link

Just in case someone else also stumbled upon this issue: https://stackoverflow.com/a/46821842/4521547

@akonradi
Copy link
Contributor

This sounds good, but we'll need to add optional support to PGS. Blocked on lyft/protoc-gen-star/pull/85

This was referenced Apr 29, 2021
Closed
Closed
@snqk
Copy link
Contributor

snqk commented May 4, 2021

Looks like this can go ahead now. 🎉

@glerchundi
Copy link
Contributor Author

Checklist updated with the PR bumping to the newly released protoc-gen-star version.

@snqk
Copy link
Contributor

snqk commented May 6, 2021

I assume for this, the templates need to be updated for sure, but are there any additional checks that would need to be added?

@snqk snqk mentioned this issue May 15, 2021
Merged
snqk added a commit to snqk/protoc-gen-validate that referenced this issue May 22, 2021
@snqk
pgs: support proto3 presence & bump go.mod (bufbuild#431)
f4fcc30 
Signed-off-by: Sarthak Gupta <signed@sarthak.sh>
@snqk snqk mentioned this issue May 22, 2021
Closed
snqk added a commit to snqk/protoc-gen-validate that referenced this issue May 22, 2021
@snqk
pgs: support proto3 presence & bump go.mod (bufbuild#431)
1291366 
Signed-off-by: Sarthak Gupta <signed@sarthak.sh>
@aantono aantono mentioned this issue Jun 21, 2021
Closed
@akonradi akonradi mentioned this issue Jul 14, 2021
Closed
@nikoncode
Copy link

nikoncode commented Jul 15, 2021 •
edited
Loading

This issue is about support for go or for java too?

@bunch-helen
Copy link

I am waiting for this support for Java

@eroshiva
Copy link

Hi all,
What is the status of this issue? Is someone working on it? When we should expect a fix for this issue?

@akonradi
Copy link
Contributor

There's been some progress (see PRs/issues linked above). It looks like #474 made some progress but needs some changes.

@snqk
Copy link
Contributor

snqk commented Aug 23, 2021

PGS v0.6.0 was released a few hours ago, which includes fundamental changes to progress this forward.

We're currently pending a few things in to close this out. Most notably:

  • updating PGS deps & fixing CI
  • support for *.cc, *.java & *.py
  • test(s)

I am planning to work on this over the weekend & see where I land.

@akonradi how feasible is it to split support for different languages into different PRs? While I'll try my best, I can't say I am overly confident with templating for some of these languages tbh.

@akonradi
Copy link
Contributor

Splitting support for different languages across multiple PRs SGTM.

@loeffel-io
Copy link

Any update on this? ✌️

@rodaine rodaine added the Enhancement Extend or improve functionality label Nov 12, 2021
@rodaine rodaine mentioned this issue Nov 16, 2021
Closed
mgebundy pushed a commit to mgebundy/protoc-gen-validate that referenced this issue Nov 18, 2021
@snqk @mgebundy
pgs: support proto3 presence & bump go.mod (bufbuild#431)
5eca99b 
Signed-off-by: Sarthak Gupta <signed@sarthak.sh>
@mgebundy mgebundy mentioned this issue Nov 29, 2021
Merged
@jrkt
Copy link

jrkt commented Dec 13, 2021

Is there any update on this? This is preventing us from using this plugin.

@mgebundy
Copy link
Contributor

Is there any update on this? This is preventing us from using this plugin.

There's a PR open, if you have any java experience and can give a hand that would help immensely :)

#537 (comment)

@jrkt
Copy link

jrkt commented Dec 13, 2021

Not enough to be able to contribute 😬 we are only concerned with the go implementation.

@loeffel-io
Copy link

Is there any update on this? This is preventing us from using this plugin.

same here

rodaine pushed a commit that referenced this issue Jan 12, 2022
@mgebundy @snqk @rmichela
Feature/add optional support (#537)
dc51e59 
* pgs: support proto3 presence & bump go.mod (#431)

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

* bump lyft/protoc-gen-start to v0.6.0

Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>

* bump protoc-gen-star to 0.6.0 in bazel dependencies

Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>

* add 'optional.proto' file to test harness

Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>

* add supported feature optional to go init

Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>

* add types/pluginpb to list of deps in BUILD

Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>

* bump protobuf in bazel deps to match go.mod version, add deps to BUILD

Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>

* bump com_google_protobuf to v3.15.3, matches go-protobuf v1.27 dependency

Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>

* update optional test to include optional int64 field within message

Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>

* add some test cases to try out

Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>

* remove optionalCases from cases

Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>

Co-authored-by: Sarthak Gupta <signed@sarthak.sh>
Co-authored-by: Ryan Michela <rmichela@salesforce.com>
@elliotmjackson elliotmjackson reopened this Oct 5, 2022
@loeffel-io
Copy link

maybe i am wrong but i think there is already optional support @elliotmjackson

@ryanrhee
Copy link

ryanrhee commented Oct 6, 2022

optionals now no longer make the plugin produce code that doesn't compile. but there are still decisions to be made and validation features to be written. e.g. should things marked not-optional be enforced to be non-null?

@elliotmjackson
Copy link
Contributor

Thank you for your input on adding proto3 optional support to protoc-gen-validate. I'd like to inform you that these capabilities are already available in protovalidate. As we look to the future with protovalidate, we have no intention of expanding support for proto3 optional fields in protoc-gen-validate.

I appreciate your understanding. I'll be closing the issue while keeping your suggestion in mind. If you have more questions or need assistance, feel free to reach out. Your engagement is greatly valued!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement Extend or improve functionality
Projects
None yet
Milestone
No milestone