-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
cfb34c2
to
727a09c
Compare
@ctreatma , sorry I clicked on Ready for Review by accident. |
4b62ed2
to
b3c7368
Compare
No worries! I was ready to take it out of draft anyway (or more to the point, I forgot I'd left it as a draft). |
b3c7368
to
24a8437
Compare
3da687d
to
4bcd6ec
Compare
## Enum | ||
|
||
|
||
* `GLOBAL` (value: `"global"`) |
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.
Looks like the enumClassnamePrefix
option isn't used in the docs, so the enum names here are wrong, but IMO that's something we can accept for now and propose a fix upstream. Alternative would be to eject the docs template and customize it but that feels unnecessary here.
…nline enums for us
4bcd6ec
to
7b30481
Compare
"inlineSchemaOptions": { | ||
"RESOLVE_INLINE_ENUMS": true | ||
}, | ||
"enumClassPrefix": true |
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.
Would it be possible to add an explicit files
key for the model template? I know it works without it, but it might be clearer for people to understand how the model code is generated, if there's a record of what template goes to what files.
I'm actually not sure how it should be specified, maybe sth like
"files:" {
"model_simple.mustache": {
"templateType": "model"
"destinationFilename": "???"
}
}
.. but I'm now reading through the oag code, and it seems that model_simple is a sub-template of the model template. So it might not even be possible to refer to a custom model_simple
expiclitly.
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 sure if that would work, but even if it does I'd be hesitant to make that config change in the scope of this PR, since nothing in this PR is dependent on custom templates.
It looks good, and I think it's a useful change. just briefly see my note above #145 (review) On a bit unrealted note, now that we know how templates work in oag, we could for example do a custom |
Before doing something like this, I think we'd need to improve how our custom templates are managed so that we can easily pull in upstream changes any time we upgrade the generator. I'm not a huge fan of our current patching process (I find it somewhat difficult to reason about and it can be tricky to fix things when patching fails)...but keeping templates up-to-date with the current patching process could look like:
|
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.
lgtm
This PR is included in version 0.22.0 🎉 |
This enables the
RESOLVE_INLINE_ENUMS
option foropenapi-generator@v7.0.0
to tell the generator to create dedicated models for inline enums instead of treating the inline enums as plain strings. Among other things, generating inline enums helps the SDK properly validate API responses againstoneOf
schemas in which each component schema has different enum values for the same property name.In addition to the
RESOLVE_INLINE_ENUMS
option, this also enables theenumClassPrefix
setting. Since the constants used in each enum are defined in the same scope, we have to turn this setting on in order to avoid conflicts between different enums that include identical values.Fixes #124