Repository navigation
feat: Support schemata for Schema parsing - #34
Conversation
| let owned: Vec<String> = schemata_str | ||
| .into_iter() | ||
| .map(|s| s.as_ref().to_owned()) | ||
| .collect(); | ||
| let refs: Vec<&str> = owned.iter().map(String::as_str).collect(); | ||
| let safe_schema = safe::SchemaMut::from_schemata(schema_str, &refs)?; |
There was a problem hiding this comment.
I wasn't sure exactly how to eliminate monomorphization for leaf crates other than performing another allocation. Help or direction if this can be optimized would be appreciated!
There was a problem hiding this comment.
that's almost exactly what I had in mind :)
I think ultimately that allocation is not very expensive considering the cost of the parsing itself, so that's just a nicety.
However I think this API should also be available in SchemaMut, and the code above with that alloc should live there.
| .ok_or_else(|| SchemaError::new("SchemaKey refers to non-existing node"))? | ||
| .as_ref() | ||
| .ok_or_else(|| { | ||
| SchemaError::new("Remapped SchemaKey refers to an unreachable node") |
There was a problem hiding this comment.
I've thought about whether these should return errors or panic here, since errors from here are only possible (I think) if my implementation is incorrect and corrupts nodes. Am open to suggestions, but currently have it as errors to prevent panicking for end users
There was a problem hiding this comment.
I think I like panic more here for clarity.
There was a problem hiding this comment.
updated to panic on crate bug instead of producing errors
99de48b to
d1daedf
Compare
d1daedf to
5acf590
Compare
| let key_remap = build_remap(&reachable_nodes); | ||
| remap_nodes(self, &reachable_nodes, &key_remap)?; | ||
| remove_unreachable_nodes(self, &reachable_nodes); | ||
| self.schema_json = None; |
There was a problem hiding this comment.
This shouldn't affect the json representation AFAICT, so I don't see why we need to lose the original json if there was any here?
There was a problem hiding this comment.
Good catch. remove unreferenced nodes should only be applicable for schemata or self built SchemaMut::from_nodes, and those wouldn't have schema_json to begin with anyways
02cac02 to
5220bdd
Compare
5220bdd to
e8c4936
Compare
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
We should add a comment why this test can't live in the usual tests folder.
| )) | ||
| })?, | ||
| ), | ||
| schema_json: if schemata_str.is_empty() { |
There was a problem hiding this comment.
schemata_str here feels hard to read: I'd expect that the type of this variable is &str, and is_empty() checks whether that &str is empty, but that's not what is happening here.
We should look for a better name.
There was a problem hiding this comment.
renamed for clarity!
c4820bd to
466ec7a
Compare
466ec7a to
176f3a5
Compare
2 participants
Resolves #30
I want to be transparent that a lot of the test cases were generated by AI (if thats ok with you) using GPT 5.4. However, all the implementation code is hand written and the tests were heavily inspected by myself and pruned for redundancy. Please let me know if you have any questions or if you have a strong stance for AI generated code. If you do not want AI generated code in your repo, I can remove the AI generated test cases and rewrite them, but I find them useful for identifying edge / test cases that I would have overlooked otherwise.