-
Notifications
You must be signed in to change notification settings - Fork 27
feat: add initial extension support #395
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
base: main
Are you sure you want to change the base?
Conversation
07c6293 to
95270ab
Compare
01172ec to
3e58a42
Compare
benbellick
left a comment
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.
Left a few comments but still haven't gone through most of the PR. Just wanted to flush what I have so far, but I will review more later. Thanks!
| prost = "0.14.1" | ||
| prost-types = "0.14.1" | ||
| # Required by generated text schemas: the typify-generated code emits | ||
| # ::regress::Regex for `pattern` validations. |
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.
Just to clarify, it looks like regress was already present before. Is this comment just clarifying why it was there in the first place?
| pub trait ProtoContext: Context { | ||
| /// Add a [SimpleExtensionUrn] to this context. Must return an error for duplicate | ||
| /// anchors or when the urn is not supported. | ||
| /// anchors or when the URI is not supported. |
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.
Can we just consistently use URN here? We have support for both in all of the other libraries and we will be dropping URI eventually.
| } | ||
| } | ||
|
|
||
| pub trait ProtoContext: Context { |
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.
As mentioned, I don't have a ton of Rust experience, so it may be that this is totally standard.
But I wonder if there's a simpler way to handle parsing that isn't so trait-heavy. AFAICT, there is only one implementor of the ProtoContext trait, which is the test fixture (lines 81-107).
Could we drop the ProtoContext trait and use a concrete type instead, then update tests to use explicit instantiations of that type? If not, what is the benefit of using this trait here?
Summary
Begins addressing #367 and #342 by adding support for parsing the types in YAML Simple Extension Files into Rustic types - with validity enforced. This includes a string text parser handling built-in types, compound types, named structs, custom types, and validated parameter constraints in the Simple Extension YAML files.
Scope
ExtensionFile,Registry,CustomType,ConcreteType) and enforces validation of those on creation / read.Key Changes
BuiltinType,CompoundType,ConcreteType,CustomTypewith Display/round‑trip support for alias and named‑struct structures.TryFrom<TypeParamDefsItem>,Parse<RawType>)ProtoContextfromContext, to distinguish between things needed for Protobuf parsing (ProtoContext)u!Name) and type variables; visits extension references for linkage bookkeeping.parsefeature includesserde_yaml;include!(extensions.in)is gated behindextensionsfeature.actions/checkoutto v4, updates Cargo dependency set, and bumps thesubstraitsubmodule.Compatibility Notes
ProtoContexton proto parsing that previously required onlyContext.extensions.innow compiled only withfeatures=["extensions"].Testing
features=["extensions"].