-
Notifications
You must be signed in to change notification settings - Fork 5
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
Contains a few corrections as well as revisions for upcoming protoc changes #16
Conversation
…hecks based on recent/upcoming changes in protoc
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -2082,6 +2096,9 @@ interpreted as an open range (i.e. inclusive) where the start and end values are | |||
|
|||
Reserved ranges for the same enum are not allowed to overlap one another. | |||
|
|||
The contents of the string literal that defines a reserved name must represent a valid | |||
identifier. In other words, they must match the production for the | |||
[_identifier_](#identifiers-and-keywords) lexical element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct in understanding that this check in protoc is just a warning for now, or did it change since the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, it is a warning in protoc
. But it will be promoted to an error in the following release.
It was agreed that the existing behavior was incorrect, so I think it's okay for this spec to vary from actual protoc
behavior if it is an acknowledged defect in protoc
. In fact, even their spec already implies that this not being a valid identifier would be a syntax error. But they wanted to give users at least one version where it was a warning, to give an opportunity to see the warning and fix sources before this became a compile error.
FWIW, buf
already treats this as an error (and we've had had no complaints from users on the discrepancy with protoc
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious about if the reserved names behavior change is really an error or if it's just a warning, but since it's not actually explicitly stated in this PR anyways I feel comfortable just giving the LGTM here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love that we're getting ahead of future protoc
releases! Nice work keeping up with G, Josh.
Just a personal thank you for improving the JSON name validation! I always felt bad about pointing users to this feature because it was so easy to make a silly mistake and only notice it much too late. |
This revises a couple of sections with extra information about
protoc
behavior:message_set_wire_format
option in proto3 syntax.This also adds some content based on upcoming changes that will be in v22 of protoc (protocolbuffers/protobuf#11349 and protocolbuffers/protobuf#10586):