Feature/YAML support #147
Feature/YAML support #147
Conversation
…son-schema into feature/yaml-provider
| public Factory() { | ||
| this.yaml = new Yaml(new JsonSchemaCompatibleConstructor()); | ||
| } |
There was a problem hiding this comment.
Let the user configure?
| public Factory() { | |
| this.yaml = new Yaml(new JsonSchemaCompatibleConstructor()); | |
| } | |
| public Factory(Yaml yaml) { | |
| this.yaml = yaml; | |
| } | |
| public Factory() { | |
| this(new Yaml(new JsonSchemaCompatibleConstructor())); | |
| } |
There was a problem hiding this comment.
Yeah, initially it was that way but I felt it may do no good. If it's not properly configured it won't be complaint with JSON Schema spec. Still not sure though - do you feel it'd be useful? Do you heave some specific case already?
I liked the idea that users can reuse their own Yaml instances if they want to but it requires many customizations to make it right
There was a problem hiding this comment.
For v2 I would have a use case: to share my instance, just to be consistent with normal parsing versus validator-parsing.
If it's not properly configured it won't be complaint with JSON Schema spec.
Is this because this factory also parses the Schema?
There was a problem hiding this comment.
Is this because this factory also parses the Schema?
it applies to instance and schema documents:
- having non-string property names would break stuff
- floating point numbers are not parsed with enough precision (all parsed to Double)
so if any document has these it will break
There was a problem hiding this comment.
ofc allowing user to modify yaml factory and giving appropriate warning is also viable solution - but I'm still not sure if that's a good idea
| AbstractConstruct constr = new AbstractConstruct() { | ||
| @Override | ||
| public Object construct(Node node) { | ||
| return new BigDecimal(((ScalarNode) node).getValue()); | ||
| } | ||
| }; | ||
| this.yamlConstructors.put(Tag.FLOAT, constr); | ||
| this.yamlClassConstructors.put(NodeId.scalar, new ConstructYamlStr()); |
There was a problem hiding this comment.
Would you mind explaining in comments why these are needed? I'm not versed in JSON/schema spec. It would be purely for my own benefit, so don't bother if don't think those comments would help future maintenance.
There was a problem hiding this comment.
I guess a java comment here can be useful. I described in previous PR comment what does this customisation achieve
| public JsonNode wrap(Object node) { | ||
| if (node instanceof SnakeYamlNode) { | ||
| return (SnakeYamlNode) node; | ||
| } else { | ||
| return new SnakeYamlNode(node); | ||
| } | ||
| } |
There was a problem hiding this comment.
I found a use case for supporting the snake YAML Node objects: I already have a Node in memory (that I loaded) and want to validate it against a schema. With this object-based validator I can't do this.
There was a problem hiding this comment.
Yeah, that's what I thought that would be a primary use case. I thought about wrapping snakeyaml nodes too but it would require conversion of nodes to Java objects. it's certainly doable but there is no exposed API for that. I checked how to achieve this and unfortunately it's not trivial, a lot of tinkering with seemingly internal lib classes. But I'm gonna revisit it
# Conflicts: # build.gradle
…ternal node representation rather than java objects. Now providing custom Yaml factory actually makes sense. The price is: - no validation for duplicate keys - no support for override (merge) syntax "<<" (plain alias/references still work)
|
# Conflicts: # build.gradle
|
No description provided.