Schema types metadata #19524
Schema types metadata #19524
Conversation
|
@mweatherley @splo Could be interested in this :) It is a first step towards bigger refactor- namely proper JSON Schema support. But that is a much bigger change and I want to clean up my code from here a lot more: https://github.com/Leinnan/bevy_remote_ext |
| /// Holds mapping of reflect data types to strings, | ||
| /// later on used in Bevy Json Schema. |
There was a problem hiding this comment.
What kind of "strings"? Are these the schemas themselves? Type paths? Might be worth clarifying.
Same goes for other mentions of "string" in this file.
There was a problem hiding this comment.
I don't have idea how to make it more clear ATM. If you have, please suggest 😅
| let t = reg.type_info(); | ||
| let binding = t.type_path_table(); | ||
|
|
||
| let short_path = binding.short_path(); | ||
| let type_path = binding.path(); | ||
| let mut typed_schema = JsonSchemaBevyType { | ||
| reflect_types: get_registered_reflect_types(reg), | ||
| reflect_types: metadata.get_registered_reflect_types(reg), |
There was a problem hiding this comment.
Hm, didn't realize this field was just called reflect_types when it really just holds simplified names of type data. I wonder if it should be renamed to reflect_type_data or type_data or something? 🤔
There was a problem hiding this comment.
I want to change the structure of schema a bit, so that if possible I would leave for next PR 😅
Co-authored-by: Gino Valente <[email protected]>
Co-authored-by: Gino Valente <[email protected]>
alice-i-cecile
added
C-Feature
|
@MrGVSV Updated with almost all feedback applied. |
| Some((id.to_string(), schema)) | ||
| }) | ||
| .collect::<HashMap<String, JsonSchemaBevyType>>(); |
There was a problem hiding this comment.
Nit: Unfortunate we have to allocate (to_string) here since we made export_types return Cow. Would converting this to HashMap<Cow<'static, str>, JsonSchemaBevyType> be too difficult/involved here? If so, we can address this another time.
There was a problem hiding this comment.
I would love to put it into the next PR and have this one merged 😅
| (reg.type_info().type_path().into(), (reg, metadata).into()) | ||
| } | ||
|
|
||
| impl From<(&TypeRegistration, &SchemaTypesMetadata)> for JsonSchemaBevyType { |
There was a problem hiding this comment.
Nit: Might be worth keeping a From impl for just &TypeRegistration which just uses the SchemaTypesMetadata::default.
There was a problem hiding this comment.
I would love to put it into the next PR and have this one merged 😅
|
@mweatherley @splo I could use second approve if you find that useful |
|
Looks good to me. 👍 I'm especially excited to see future PRs for proper JSON Schema support. |
|
@alice-i-cecile can it be merged? |
alice-i-cecile
added
S-Needs-Review
|
I don't have experience with this sort of thing; let me wrangle up some reviews :) |
There was a problem hiding this comment.
I’m not too familiar with the details/implementation here, but I think we should have some proper examples of serialising/deserialising with these schema changes. I’m happy that @splo is happy with these changes too :)
alice-i-cecile
added
S-Ready-For-Final-Review
|
Ah, I missed |
Objective
reflectTypesmappings for Bevy JSON Schema.Solution
SchemaTypesMetadataResource that will hold mappings forTypeIdofTypeData. List is bigger from beggining and it is possible to expand it without forking package.Testing