Skip to content

WIP: respects DefaultVal in schema and adds default value to schema. fixes #679, #396#680

Open
autoantwort wants to merge 4 commits into
getml:mainfrom
autoantwort:feature/default-in-schema
Open

WIP: respects DefaultVal in schema and adds default value to schema. fixes #679, #396#680
autoantwort wants to merge 4 commits into
getml:mainfrom
autoantwort:feature/default-in-schema

Conversation

@autoantwort

@autoantwort autoantwort commented Jun 8, 2026

Copy link
Copy Markdown

fixes #679, #396

Comment thread include/rfl/parsing/NamedTupleParser.hpp Outdated

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread include/rfl/parsing/NamedTupleParser.hpp Outdated
Comment thread include/rfl/parsing/NamedTupleParser.hpp Outdated
Comment thread include/rfl/json/schema/Type.hpp Outdated
@liuzicheng1987

Copy link
Copy Markdown
Contributor

@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:

#681

This fixes #679, but still needs some adaptions for some non-JSON formats.

@liuzicheng1987

Copy link
Copy Markdown
Contributor

@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.

@autoantwort

Copy link
Copy Markdown
Author

@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.

I didn't know you were going to propose a fix, so I started working on my own: #681

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?

@liuzicheng1987

Copy link
Copy Markdown
Contributor

@autoantwort, I think I can continue on mine to fix #679 and then we can dedicate this PR to fixing #396

@liuzicheng1987

Copy link
Copy Markdown
Contributor

@autoantwort I have just merged my PR which fixes #679, you can merge main into this branch and then continue working on #396.

@autoantwort autoantwort force-pushed the feature/default-in-schema branch from 0d57407 to 1290a77 Compare June 10, 2026 15:01
@autoantwort

Copy link
Copy Markdown
Author

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rfl::DefaultVal's are still required in the json schema

2 participants