-
Notifications
You must be signed in to change notification settings - Fork 4
Add MetaDescriptor and Json Schema converter #83
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
Add MetaDescriptor and Json Schema converter #83
Conversation
SPC-code
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.
Please add general MR description and at least one example of usage of schema.
dataforge-meta/build.gradle.kts
Outdated
| sourceSets { | ||
| val commonTest by getting { | ||
| dependencies { | ||
| implementation("io.github.optimumcode:json-schema-validator:0.5.2") |
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.
Please move it to kscience.commonTest block.
| import kotlinx.serialization.json.* | ||
| import space.kscience.dataforge.meta.* | ||
|
|
||
| public object MetaDescriptorJsonSchemaConverter { |
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.
Please document all public classes, objects and methods.
| import space.kscience.dataforge.meta.* | ||
|
|
||
| public object MetaDescriptorJsonSchemaConverter { | ||
| public fun convert(metaDescriptor: MetaDescriptor): JsonObject { |
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.
Better to give different names for forward and backward conversion
| /** | ||
| * Convert [MetaDescriptor] to a JSON Schema [JsonObject] | ||
| */ | ||
| public fun MetaDescriptor.toJsonSchema(): JsonObject { |
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.
Does it make sense to make converter private or avoit namespace object at all?
| } | ||
|
|
||
| // Assert | ||
| assertTrue(result.isSuccess, "Expected no exception but got ${result.exceptionOrNull()}") |
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.
This is very strange test design. You can just run function. It will throw an exception if test fails.
| node(name, childDescriptor.applyRequiredRestrictions()) | ||
| } | ||
| } | ||
| } No newline at end of file |
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.
Aree there any test to actually test conversion correctness?
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.
The current tests already validate:
- Schema validity -
testIsJsonSchemaValidchecks that the generated JSON Schema is syntactically correct (viaJsonSchema.fromJsonElementfrom libraryio.github.optimumcode:json-schema-validator) - Round-trip conversion –
testIsSerializationAndDeserializationWorksCorrectensuresdescriptor → schema → descriptorproduces an equivalent structure
If you’d like additional validation of specific schema properties, let me know - I can add targeted test cases.
MetaDescriptor and JSON Schema Conversion
The implementation provides bi-directional conversion between
MetaDescriptorand JSON Schema (asJsonObject) formatsExample Usage:
Resulting JSON Schema representation:
{ "$schema": "https://json-schema.org/draft/2020-12/schema", "required": [ "user" ], "properties": { "user": { "title": "user", "required": [ "name" ], "properties": { "name": { "title": "name", "description": "User full name", "type": "string", "__indexKey__": "@index", "__multiple__": false, "__attributes__": {} }, "age": { "title": "age", "type": "number", "default": 18, "__indexKey__": "@index", "__multiple__": false, "__attributes__": {} } }, "__indexKey__": "@index", "__multiple__": false, "__attributes__": {} } }, "__indexKey__": "@index", "__multiple__": false, "__attributes__": {} }Related Issues:
closes #35
closes #KS-522