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

Support default values for schema properties #649

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PARAIPAN9
Copy link
Contributor

Motivation

Modifications

  • Update schema translations.

Result

  • Schema translations will support default values.

Test Plan

  • Run test suite. TODO - Write more tests.

@PARAIPAN9
Copy link
Contributor Author

Opened it as a draft. Not sure if I addressed correctly all the schema translations. I'd appreciate your input so far.

@czechboy0
Copy link
Contributor

Hi @PARAIPAN9,

thanks for starting on this work! I think roughly the direction is good, but it's difficult for me to fully understand the impact of the changes on the generated code.

I wonder if we could flip it, and first just write a bunch of snippet tests (that'll fail, initially), that show what code we want generated? That's where we can discuss which defaults we want to support, and which ones we might want to hold off on for now. I'm seeing defaults for things like typealiases and arrays and I realize I'm not sure I know how those should work.

Note that you might also want to add a sublist under default here:

Write out all the schema types we support in general, and have an empty box for each of them. Then we can better track which schemas we have a plan/implementation for, and which ones don't support defaults yet.

How does that sound?

@PARAIPAN9
Copy link
Contributor Author

Sounds perfect @czechboy0!! Thank you very much for your guidance!!

@PARAIPAN9
Copy link
Contributor Author

I've created this table to show you how defaults would be generated according to the changes from this PR to give you a better undestanding. Feel free to comment.

OpenAPI Document Generated Code Comments
Object Schema objectSchemaOpenAPIDoc objectSchemaGenerated ...
Array Schema arrayOpenAPIDoc arrayGenerated ...
Primitive Schemas typealiasOpenAPIDoc typealiasGenerated ...

@simonjbeaumont
Copy link
Contributor

The object schema defaults look sensible (default parameter for the init).

For the other two, the static default property is on Components.Schemas which is probably wrong. How would multiple of these compose? Also, because we're generating type aliases for these, it's not even clear to me where the default should go.

It wouldn't make sense, for example, to add an extension because we'd be adding an extension to e.g. [Swift.Bool]. Had we been generating nominal types for these, we might have been able to do it.

What I was thinking is that we don't generate this static property but we propagate the default value from the document to the places where the type is used as a parameter. E.g. anything that was generating a parameter of this type would continue to do so, but also include a default value for that parameter.

WDYT?

@PARAIPAN9
Copy link
Contributor Author

Yes, it makes sense. Thanks!!

@czechboy0
Copy link
Contributor

We already have something similar for optionality of values - they affect the call site of the type, rather than the definition of the type. So there's some precedent at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support default values for schema properties
3 participants