-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
test(go): add test for RESOLVE_INLINE_ENUMS #21623
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
base: master
Are you sure you want to change the base?
test(go): add test for RESOLVE_INLINE_ENUMS #21623
Conversation
also on the note of a "breaking" change. It is breaking and kind of not: The enums have the base type as before so from a user perspective a missing cast is required if the base types are being used. The non-breaking part is that it is still the same more or less. For that reason I was unsure if it is now breaking or non breaking. |
@wing328 is anyone of the Go Maintainers still active? |
have another issue to fix regarding xml instructions being completely ignored but if none of the maintainers react it is not worth putting time into that... |
your PR is still a draft that's why I've not yet found the time to review this change. can you please PM me via Slack if you need further assistance to get this PR ready to be reviewed? |
as you mentioned, if you've the option enabled, it works fine for your use cases or still an issue with the option enabled? |
@wing328 all good, the question was directed at the fact that none of the go maintainers tried to look at the PR. I set the PR to read for review just now |
can you please review the build failure when you've time? |
@@ -113,6 +113,9 @@ public GoClientCodegen() { | |||
outputFolder = "generated-code/go"; | |||
embeddedTemplateDir = templateDir = "go"; | |||
|
|||
// We need inline enums to be resolved to a separate model | |||
inlineSchemaOption.put("RESOLVE_INLINE_ENUMS", "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.
what about checking if the key RESOLVE_INLINE_ENUMS
is already set (true or false) before defaulting it to true so that users have a way to fallback?
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.
sure, how?
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.
what about using containsKey method?
also let's not do this in the constructor
please do it in processOpts instead
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.
ok, will do once we have a con sense how to handle the default situation.
sure. It fails because of #21623 (comment) |
if enum is just a model, then the modelNameMapping option may help https://github.com/openapitools/openapi-generator/blob/master/docs/customization.md#name-mapping |
I think for this PR I can change one of the enums to WDYT? |
let me think about it ... is it correct to say that the option |
as far as I can remember I had to "bake" the option in the code like you can see in the PR as I couldn't figure another way to make it work. You can try to outcomment line 117 and let the build run. |
also looking at the changes, it seems that the "default" I set with that now enables enum Generation in other places so that is where the build failure comes from. |
yes, it helps using |
Not intentionally, I personally would but I have no idea what the rationale is to not have them generated. Is there a ADR or some issue for that (or a general design decision)? So at the moment this PR is boiling down to one added test proving that the inline enums are working in general and highlighted an important issues (Think we should file one for that!?) regarding enums not being prefixed with their type name and hence producing nasty name collisions. The model_name_option you mentioned is IMHO not feasible as such collisions are expected to be quite common. |
3eb74a2
to
d6ef5b1
Compare
(force-push because I could not get rid anymore of the generated code which was just not correct anymore after the latest adjustments) |
backward compatibility refactoring the enum into models will break those using the auto-generated Go SDK. |
Closes #9567
The PR added a unit test to demonstrate missing enums as defined in #9567. To validate one can uncomment line 117 in
GoClientCodegen.java
and then the tests will fail.@antihax @grokify @kemokemo @jirikuncar @ph4r5h4d @lwj5
PR checklist
Commit all changed files.
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*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming7.x.0
minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)