Skip to content
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

Consider making isInliningPropertiesFromReferencedSchemas the default behaviour #63

Closed
Tracked by #101
liamnichols opened this issue Jul 28, 2022 · 3 comments · Fixed by #137
Closed
Tracked by #101
Labels
breaking Results in a breaking change in behaviour bug Something isn't working help wanted Extra attention is needed
Milestone

Comments

@liamnichols
Copy link
Member

Background

Context: #61 (comment)

When isInliningPropertiesFromReferencedSchemas is not enabled (the default), it generates an invalid Encodable implementation. But generally speaking, the nested objects seem to not be consistent with how other generators generate code and it leads me to think that it's not the right choice to be nesting entities.

My proposal is that we remove the isInliningPropertiesFromReferencedSchemas option and make it's behaviour the default.

@liamnichols liamnichols added bug Something isn't working help wanted Extra attention is needed labels Jul 28, 2022
@kean
Copy link
Member

kean commented Jul 28, 2022

I think it's a good idea to make this behavior the default. Looking at the example, if this I'm not sure if the Pet even needs to be generated in this case (assuming it's only ever used in allOf and never referenced as a separate entity).

@kean
Copy link
Member

kean commented Jul 28, 2022

I would suggest keeping this option because there are generally three types of ways to address it: composition, inheritance, and inlining all properties. CreateAPI covers everything but inheritance right now.

One more thing CreateAPI could do when inlining is used, is generate Pet as a protocol and add conformance to it to the types like Dog.

@LePips
Copy link
Contributor

LePips commented Jul 28, 2022

not sure if the Pet even needs to be generated
generate Pet as a protocol

I've just started reading OpenAPI to learn but it seems more safe to inline or protocol as an inheritance hierarchy cannot necessarily be assumed with allOf.

I can think of cases where people want either of these generation options. We would have to make sure that entity generation doesn't follow isMakingClassesFinal.

# Apply to all objects
# default: inline
parentObjectGeneration: entity || protocol || inline

# Specify per type
# default: inline
parentObjectGeneration:
  - Pet: protocol
  - Human: entity

Edit: don't do that. We should think about how discriminators would create inheritances but I think only with protocols. https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Results in a breaking change in behaviour bug Something isn't working help wanted Extra attention is needed
Projects
None yet
3 participants