-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Add swift5 option for generating frozen enums #11013
Add swift5 option for generating frozen enums #11013
Conversation
{{#allowableValues}} | ||
{{#enumVars}} | ||
case {{{name}}} = {{{value}}} | ||
{{/enumVars}} | ||
{{/allowableValues}} | ||
{{^generateFrozenEnums}} | ||
{{#vendorExtensions.x-non-frozen-enum-capable}} | ||
case unknown |
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.
@jarrodparkes I'm afraid this will conflict with an Enum case that is already unknown
, for example, in my project some of the Enum's already have a unknown case.
Here are some alternative names:
- unknownDefault (@unknown default)
- openApiUnknown
- {projectName}Unknown
Do you have other suggestions?
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.
@4brunu good call. of your options, I like unknownDefault
personally
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.
@4brunu updated
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.
Sorry for didn't put more thought into it, but since I discover that the raw value name is the case name, maybe we should name the enum something a bit more complex to be extra sure that we avoid conflicts.
What do you think of the following suggestions?
- {projectName}UnknownDefault
- unknownDefault{projectName}
- openApiUnknownDefault
- unknownDefaultOpenApi
Do you have other suggestions?
I also like the unknownDefault because of the pattern in Swift @unknown default, but maybe we should try harder to avoid conflicts?
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.
Yeh, probably so. I like unknownDefaultOpenApi
. And now that I think about it, enum cases that are multiword strings, normally use snake_case. With that in mind, I can also set String raw value to "unknown_default_open_api"
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 good to me 👍
{{#allowableValues}} | ||
{{#enumVars}} | ||
case {{{name}}} = {{{value}}} | ||
{{/enumVars}} | ||
{{/allowableValues}} | ||
{{^generateFrozenEnums}} | ||
{{#vendorExtensions.x-non-frozen-enum-capable}} | ||
case unknown |
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.
@jarrodparkes same here
Here are some experiences that I was doing
|
^ @4brunu the updated As for
|
What about something like |
@4brunu It has to be a literal value. |
|
||
if (property.isEnum) { | ||
// A non-frozen (with the "unknownDefault" case) enum is only possible with String or Int raw types. | ||
if (property.dataType.equals("String") || property.dataType.equals("Int")) { |
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.
Since now we are going to assign values to the enums, can we remove this?
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.
Maybe. Let me push the next update. I think we'll want to know the enum's type in order to assign a default value that is correct.
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.
Is there any enum type that we don't support?
I think there is an easier way to do this, by removing this block of code and do this type check in the mustache files.
I think there are checks like isString, isInt, isDouble, isNumber, but I need to check tomorrow, because I'm not 100% sure.
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.
@4brunu You are correct. I had tried this earlier, but was using the mustache clauses incorrectly. I made the change to remove the custom vendor extensions and use the built-in type checks. I think we should be good now
That number doesn't fit inside the Int range. |
@4brunu alright, take a peek now 👀 and let me know what you think |
Fix #1529 |
// https://openapi-generator.tech | ||
// | ||
|
||
/// An enum where the last case value can be used as a default catch-all. |
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.
There are some protocols already defined in the models.mustache files, I think, so I'm not sure if we should create a new file or put this one with the others that already exist.
Although this is not a very important topic, and it's subjective, but I'm just trying to keep things simple and coherent.
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.
Here is an example
openapi-generator/modules/openapi-generator/src/main/resources/swift5/Models.mustache
Lines 9 to 15 in 9280e55
protocol JSONEncodable { | |
func encodeToJSON() -> Any | |
} | |
extension JSONEncodable { | |
func encodeToJSON() -> Any { self } | |
} |
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.
that's fair — I wasn't really sure what was recommended or preferred. I don't have a preference, so I'm fine to move it — will take care of shortly
{{#allowableValues}} | ||
{{#enumVars}} | ||
case {{{name}}} = {{{value}}} | ||
{{/enumVars}} | ||
{{/allowableValues}} | ||
{{^generateFrozenEnums}} | ||
{{#vendorExtensions.x-non-frozen-enum-capable}} | ||
case unknownDefaultOpenApi{{#vendorExtensions.x-non-frozen-enum-int}} = -11184809 // -(Int.min / 192){{/vendorExtensions.x-non-frozen-enum-int}}{{#vendorExtensions.x-non-frozen-enum-string}} = "unknown_default_open_api"{{/vendorExtensions.x-non-frozen-enum-string}} |
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 like that you put the comment here, and I think we should expand it a bit so that it's explicite on why this case is here.
And could you please also add a comment on the case above please?
The comment could maybe be something like:
This enum case is a fallback generated by OpenAPI, in case the server adds new enum cases that are unknown by old versions of the client. The raw value of this case is a dummy and it tries to use a value that avoids collisions.
And if its a number, put the precious message and also add the following:
The formula used to generate it is Int.min/192 (the Swift Evolution issue number for frozen/non-frozen enums)
Note: English is not my main language, and this message ended up being very big, so fell free to improve it, shorten it or write a completely different one.
This is just something that I thought that may help the users of this library understand this new enum case.
Or maybe this is overkill and unnecessary?
What do you think?
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 like it, will add shortly 😄
{{#allowableValues}} | ||
{{#enumVars}} | ||
case {{{name}}} = {{{value}}} | ||
{{/enumVars}} | ||
{{/allowableValues}} | ||
{{^generateFrozenEnums}} | ||
{{#isString}} |
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.
@jarrodparkes should we support more types? Maybe Double, Float, and more?
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.
now that we are specifying the raw values, yes 😄 — will add shortly
By the way, thanks for creating this PR, this is a big improvement on the Swift Generator 👍 |
I forgot one thing, since you added a new sample project, please make the following changes:
This will add the sample project to the CI pipeline. |
I think that covers your suggestions from this morning. The one change I felt iffy about was the use of It was as-if some enums with // NOTICE: This was marked as {{#isString}}
public enum JustSymbol: String, Codable, CaseIterable, CaseIterableDefaultsLast {
case greaterThanOrEqualTo = ">="
case dollar = "$"
// This case is a catch-all generated by OpenAPI such that the enum is "non-frozen".
// If new enum cases are added that are unknown to the spec/client, they are safely
// decoded to this case. The raw value of this case is a dummy value that attempts
// to avoids collisions with previously specified cases.
case unknownDefaultOpenApi = "unknown_default_open_api"
}
// NOTICE: This was marked as {{#isContainer}}
public enum ArrayEnum: String, Codable, CaseIterable, CaseIterableDefaultsLast {
case fish = "fish"
case crab = "crab"
// This case is a catch-all generated by OpenAPI such that the enum is "non-frozen".
// If new enum cases are added that are unknown to the spec/client, they are safely
// decoded to this case. The raw value of this case is a dummy value that attempts
// to avoids collisions with previously specified cases.
case unknownDefaultOpenApi = "unknown_default_open_api"
} |
Hum, I see, I don't have a better solution for that :/ |
I guess the greater question is... Is using the |
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 think this is the last small nits
// decoded to this case. The raw value of this case is a dummy value that attempts | ||
// to avoids collisions with previously specified cases. | ||
{{#isString}} | ||
case unknownDefaultOpenApi = "unknown_default_open_api" |
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.
@jarrodparkes this is my fault, and sorry for that, but it should be unknownDefaultOpenAPI
, of OpenAPI
// | ||
{{/isNumeric}} | ||
{{#isInteger}} | ||
case unknownDefaultOpenApi = -11184809 // -(Int.min / 192) |
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.
@jarrodparkes typo here, the formula is Int.min / 192
, and not -(Int.min / 192)
case unknownDefaultOpenApi = -11184809 // -(Int.min / 192) | ||
{{/isInteger}} | ||
{{#isDouble}} | ||
case unknownDefaultOpenApi = -0.016362461737446838 // -(Double.pi / 192) |
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.
@jarrodparkes maybe it's a good idea to use always the same number -11184809 in all the cases?
case unknownDefaultOpenApi = -0.016362461737446838 // -(Double.pi / 192) | ||
{{/isDouble}} | ||
{{#isFloat}} | ||
case unknownDefaultOpenApi = -0.01636246 // -(Float.pi / 192) |
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.
@jarrodparkes same here
{{#allowableValues}} | ||
{{#enumVars}} | ||
case {{{name}}} = {{{value}}} | ||
{{/enumVars}} | ||
{{/allowableValues}} | ||
{{^generateFrozenEnums}} |
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.
@jarrodparkes please apply all the feedback from modelEnum.mustache
also to this class
@jarrodparkes looking at the modelInlineEnumDeclaration.mustache, looks to me that the use of Line 1 in 0e873a9
|
{{#isString}} | ||
case unknownDefaultOpenApi = "unknown_default_open_api" | ||
{{/isString}} | ||
{{#isContainer}} |
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.
@jarrodparkes should this logic of the {{#isContainer}}
be added to the file modelEnum.mustache
?
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 don't believe so — the only thing that gave me the hunch to use {{#isContainer}}
on modelInlineEnumDeclaration.mustache
was the fact that I already saw that clause used earlier in the file to help construct the enum's raw type. If you look at modelEnum.mustache
, no such clause exists.
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.
You are right 👍
@4brunu made the other changes 👍 |
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 good to me 👍
Thanks, this e a great improvement to the OpenAPI Swift Generator 🙂
@jarrodparkes This PR was awesome, so I created #11078 that makes the enum unknown case available to all generators. |
Attempts to solve #1529
This PR adds a new
swift5
option forgenerateFrozenEnums
(default: true). If set to false, then qualifying enums (those with raw type ofString
orInt
), will includecase unknownDefault
and be "non-frozen" — able to "support" cases not explicitly specified in the spec.PR checklist
Core Team Members
@jgavris
@ehyche
@Edubits
@jaz-ah
@4brunu