Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add buf.registry.descriptor.v1 package #102
base: main
Are you sure you want to change the base?
Add buf.registry.descriptor.v1 package #102
Changes from 4 commits
e2deecc
2b476f0
514a6e5
a08a12b
2a81195
4cca335
52b59ac
b68d822
6aa1167
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 found the middle paragraph here a bit of a tangent that was confusing on first read. Wondering if we can resequence this documentation so the behavior is documented first, and then the "why" is last (if necessary at all).
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.
Additional design note - this was a
bool syntax_specified
inimage.proto
, but it seems clearer to make this the actual specified syntax, if it exists.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.
Do we want to validate this with an
in
constraint?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.
So for a proto file with
syntax="proto2"
, or without a syntax specified,FileDescriptorProto.syntax
is blank, and this field is"proto2"
.What happens for a file with
edition="2023"
?FileDescriptorProto.syntax
will be"editions"
in this case, but syntax wasn't really specified. Is this field""
, or"editions"
?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.
Late to the party, but this is the most important thing to me: the RPC should be side effect free, so we can expose it as an HTTP GET using Connect.
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.
Open question whether this should be
include
orexclude
- we decided for the HTTP GET endpoint to make thisinclude
.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.
In my mind, the decision on whether to call this
include
orexclude
depends on whatever you want to be returned by default. If this should be returned by default, thenexclude
is right (opt-out). If it shouldn't be returned by default, theninclude
is right (opt-in). To decide what the default would be, I would think about the anticipated use cases and whether a majority of those use cases would need or not need this info. (similarly for the other fields)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.
Writing tools against descriptors, I find myself constantly forgetting that the info is not returned by default and I need to set a flag somewhere in the chain of tools to ensure the info is available. I'd rather need to ask for it to be stripped as an optimization than have to remember to include it, personally.
This goes for the other opt-in/out questions below, as well.
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.
Including by default is more convenient for use cases like the CLI. Excluding by default is better for constrained environments, where things suddenly stop working because a giant module with many comments shows up.
Giant modules have other downsides. Including by default is the least surprising behavior in my book.
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.
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.
Open question whether this should be
include
orexclude
- we decided for the HTTP GET endpoint to make thisinclude
.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.
Open question whether this should be
include
orexclude
.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.
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.
Does this imply we're doing something new here? I thought we used to always set the version to nil/unset since version of the Buf CLI weren't really comparable to versions of
protoc
.