Add editions support to protoprint #600
Conversation
| if syn == "editions" { | ||
| _, _ = fmt.Fprintf(w, "edition = %q;", strings.TrimPrefix(fdp.GetEdition().String(), "EDITION_")) | ||
| return | ||
| } |
There was a problem hiding this comment.
I think this is supposed to go below line 538, right? Otherwise, syn is not yet defined here.
BTW, you can verify everything in a local checkout by running make ci.
There was a problem hiding this comment.
doh, I had to copy this by hand and got the lines mixed up :P. Thanks for the test command :)
There was a problem hiding this comment.
looks like you might be on an older version of go-protobuf too, IsClosed doesn't work :(
There was a problem hiding this comment.
ah this will likely have to wait for a new release
There was a problem hiding this comment.
This is the commit we have if you have another idea: protocolbuffers/protobuf-go@3039476
There was a problem hiding this comment.
Figured that out, now I'm hitting protocompile's error:
print_test.go:268: Unexpected error: test.proto:2:11: editions are not yet supported; use syntax proto2 or proto3 instead
There was a problem hiding this comment.
For protocompile, you can do:
go get google.golang.org/protobuf/proto@3039476726e4 && \
go get github.com/golang/protobuf@v1.5.4 && \
go mod tidyAs for the protocompile issues, I was hoping to cut a release this week with editions support. The last thing I have in progress is a branch to add validation of feature usage, and then it should be okay. (Though I haven't done anything with the just-added FeatureSupport stuff. That is, unfortunately, not yet available in the protobuf-go descriptorpb package.) Right now, all of its editions support is test-only (behind an internal flag that only tests can flip on).
Until then, I think you could add the example file to internal/testprotos and then update internal/testprotos/make_protos.sh to generate a descriptor set with protoc. The test could then read that descriptor set and use protodesc.NewFiles to turn the descriptor set into protoreflect.FileDescriptor instances, which you can then wrap and print.
Sorry that's a bit convoluted. If that's a bit much, I'm happy to handle all of that. Just let me know if you want me to poke at that list of steps or if you will try to do it.
There was a problem hiding this comment.
hmm looks like this might need protoparse updates too to handle the reserved grammar changes in editions :)
I think I got all this working but the AST test tries to parse the new editions file and dies in convertASTReserved
There was a problem hiding this comment.
might need protoparse updates too to handle the reserved grammar changes in editions
Not exactly. The protoparse package uses protocompile under its hood, and protocompile is almost done with editions changes, and includes support for reserved identifiers. See the most recent pull request, which is the last thing to add to protocompile for editions support (well... the last thing before the new FeatureSupport stuff landed this week): bufbuild/protocompile#264.
The problem is that protocompile rejects editions files right now -- the support for it is only enabled in tests (with the idea being that we enable it outside of tests, for users, once the work is done).
For now, we need to just conditionally turn off the verification using protoparse for this particular test file. And we can instead rely on the desc/protoprint/testfiles/check-protos.sh script to verify the golden outputs are valid. That script will need to be updated to use the --experimental_editions flag in its protoc invocation so it doesn't fail on that file.
No description provided.