-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat(dynamite)!: add experimental standalone JsonSchema generation #1964
base: main
Are you sure you want to change the base?
Conversation
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.
First commit message has a typo and should be feat(dynamite): do not rely on the openapi spec for type resolving
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.
Quite a bit of this code is just copied from the existing builder and should be factored out into common methods.
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 not so sure how much we can deduplicate this without making a mess of the code.
I'll take another look on what makes sense.
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 not a schema definition: https://json-schema.org/learn/getting-started-step-by-step#create
We should really implement the whole thing and not just the schema part itself.
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 don't understand? what is still missing?
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 metadata that can be inside a schema definition is not considered in the generated code. For OpenAPI specs we add title, description, license etc. to the top of the file while this doesn't seem to happen for JSON schema definitions afaict.
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.
Sorry my initial message was a bit confusing, I'd simply like to have proper support for JSON schema definitions.
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.
Can you please say what JsonSchema keywords you'd like to support?
It appears that the only user facing information is the title
and I don't think that it should be set as a library level doc comment. I'd rather add it to the generated class itself similar to how the dart doc style expects the documentation:
/// {title}.
///
/// {description}.
class MyObject {}
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.
Yes the comment should be on the generated class. IMO the $id
can also be relevant for users, so I'd like to have that linked as well.
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.
Since there is $defs
it is possible to have multiple object inside a single schema. I don't know if the title
and description
should be interpreted as the title and description for the main object or the whole schema.
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.
In theory we already support $defs
. our reference resolving is independent from the dart schema object and directly searches in the json data (see: #1953).
Therefore only the schemas referenced in the main schema will be generated. Or do you mean to generate all schemas defined in the definitions just in case 🤷♀️
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.
No this was about the other fields, not specifically $defs. I was just saying that title and description might not be for the main schema but for the whole schema which would make them library level.
Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
…on in ofs generation Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
…ng on JsonObject Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
44e594b
to
a1705e0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1964 +/- ##
==========================================
+ Coverage 28.43% 29.31% +0.87%
==========================================
Files 245 245
Lines 81449 76519 -4930
==========================================
- Hits 23161 22430 -731
+ Misses 58288 54089 -4199
*This pull request uses carry forward flags. Click here to find out more. |
Making a draft for now until we have merged all the depending changes. |
9658bf4
to
0b64d23
Compare
|
@Leptopoda can we move this forward and include it in the next batch of releases or is there something holding it back? |
I don't think this is anything we can immediately implement.
This requires a complete rework of how our |
implements: #933