-
Notifications
You must be signed in to change notification settings - Fork 66
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
Implement JsonSchemaAs for KeyValueMap #713
Conversation
The CI errors all seem to be related to the |
65609d6
to
16a1908
Compare
This one is probably the most complicated of all the schema adaptors. We have to process the inner schemas until we find the one with the `$key$` field, remove that field, and then recreate it as an enum.
16a1908
to
a804257
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #713 +/- ##
==========================================
+ Coverage 65.64% 66.04% +0.40%
==========================================
Files 38 38
Lines 2343 2421 +78
==========================================
+ Hits 1538 1599 +61
- Misses 805 822 +17 ☔ View full report in Codecov by Sentry. |
impl<T, F: FnOnce(T)> Drop for DropGuard<T, F> { | ||
fn drop(&mut self) { | ||
// SAFETY: value is known to be initialized since we only ever remove it here. | ||
let value = unsafe { ManuallyDrop::take(&mut self.value) }; |
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.
Small nit: I think you can use a Option
for the value. Then you take()
the option in the drop
implementation. The Deref
implementation would need a match with panic, in case the Option
is None
, but it should be possible to get rid of the None
.
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'm occasionally a little prone to using unsafe
for stuff like this. If you would prefer that I use an option here I can definitely submit a PR to change that.
Thank you, this looks good. Your comments are good, explain what is going on, and I could follow them. Thanks for getting this to work :) |
I'm back with yet another PR! This time we're doing
KeyValueMap
.I think this one is probably the most complicated conversion that needs to be done to make the schema work. We need to convert a schema that looks like this
into this
On its own that is not to bad but it gets more complicated since we also have to deal subschemas which can be generated from types that make use of
#[serde(untagged)]
and#[serde(flatten)]
. See the doc comment onkvmap_transform_schema
for more details.Notes
anyOf
subschema that makes use ofminProperties
andmaxProperties
(normal ones where they have a$key$
field should work fine). I'm not entirely sure there is one correct behaviour in the case either but to me this seems like a edge case that is pretty far out there so it should be fine.$key$
property ends up being defined in some external schema (i.e.$ref
points to some random document on the internet). This seems like an edge case and I don't think there's any reasonable behaviour here anyway.