WIP: respects DefaultVal in schema and adds default value to schema. fixes #679, #396#680
WIP: respects DefaultVal in schema and adds default value to schema. fixes #679, #396#680autoantwort wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for generating default values in JSON schemas by adding a WithDefault schema type and updating the parsers to extract and serialize default values. However, several critical issues were identified in the review: a compilation failure occurs when a struct contains a mix of DefaultVal and non-DefaultVal fields due to invalid .get() calls on standard types; template deduction and type conversion issues with (void*) nullptr will cause compilation errors in NamedTupleParser::to_schema; and storing default values as std::string leads to double-serialization (e.g., "default": "10" instead of "default": 10) in the output schema.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@autoantwort, I don't think it's a good idea to be trying to fix two problems at once and fixing #396 is tricky. There is a reason no one really took it up in years. Also I am very concerned about the proposed the solution of passing a void ptr. I didn't know you were going to propose a fix, so I started working on my own: This fixes #679, but still needs some adaptions for some non-JSON formats. |
|
@autoantwort, if you want to fix #396, I think the best way would be to transform the struct with the default values into an rfl::Generic and then pass that to the to_schema function. That might actually work. |
Yeah that worked.
Ok so you want to continue yours and I than rebase this on yours or you use the idea from here in #681 or in a new PR? |
|
@autoantwort, I think I can continue on mine to fix #679 and then we can dedicate this PR to fixing #396 |
|
@autoantwort I have just merged my PR which fixes #679, you can merge main into this branch and then continue working on #396. |
0d57407 to
1290a77
Compare
|
Done |
fixes #679, #396