-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[C#] Emit Default is true for required properties. #11607
[C#] Emit Default is true for required properties. #11607
Conversation
Partial fix for OpenAPITools#11008. A full fix requires using nullable value types.
a853fb9
to
81881d3
Compare
It looks like the failures here were both network issues (to maven and github) rather than test failures - could someone with access re-execute them to verify? Node 0:
Node 2:
|
Thanks @hauntingEcho. (@ any potential reviewers -) In terms of anyone able to review - the key files are the mustache files, and the key context is in my original message. Happy to rebase / re-generate if/when I've got a supportive review. More generally - I'm potentially keen / happy to help with fixing issue 1 in my original message - or creating a new variant from the ground up which would fix 1 and various other bugs I've encountered (I've worked around about 10 or so distinct issues so far) - but I'd like to discuss with someone who could review/approve first, to check any effort would be appreciated. |
Closing as this fix was included in #13049 |
Partial fix for #11008. A full fix requires using nullable value types.
Details
There are two issues:
This PR fixes case 2 - and sets
EmitDefaultValue
to true for required types (the default value for this attribute if it's not present). Note - this does lead to potentially emitting null for required non-nullable reference types - but the type is asserted to be non-null in the constructor anyway, so this shouldn't arise in practice.Another approach would be to simply not output EmitDefaultValue at all if the field is required (which would default to emitting defaults).
A good example to see (1) and (2) is Name.cs post this fix.
NOTE: #11280 fixes 1 and 2 by making EmitDefaultValue true just for all value types; but it will cause a new issue: non-required parameters which aren't set will output 0 when they shouldn't.
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
(5.3.0),6.0.x
- *It appears master is now 6.0.0-SNAPSHOT` already...