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

Non-optional enum properties with only variant should set a default value #169

Open
aaronsky opened this issue Nov 23, 2022 · 6 comments · May be fixed by #170
Open

Non-optional enum properties with only variant should set a default value #169

aaronsky opened this issue Nov 23, 2022 · 6 comments · May be fixed by #170

Comments

@aaronsky
Copy link

Hello – I'm working on an App Store Connect API client and have found your library very useful for processing the OpenAPI schema. One thing I've noticed that I believe would improve things for my library users would be to simplify these kinds of cases:

public struct App: Codable, Hashable, Identifiable {
    public var type: `Type`
    ...

    public enum `Type`: String, Codable, CaseIterable {
        case apps
    }

Here we have a struct with a non-optional property, referencing an enum with only variant. As a library author and user I would like to minimize the amount of repetition at the callsite, and one way to do that would be to add a default value for this variant in the initializer. Something like this:

public init(type: `Type` = .apps, ...) {

Is that a reasonable enhancement request? I may submit a patch if it isn't too difficult to do.

@liamnichols
Copy link
Member

Thanks for the request!

A quick question before digging deeper... Does the ASC schema happen to specify a default value for these properties? There is already the entities.includeDefaultValues option but it only supports Bool at the moment.

I guess the first question that came to my mind was that maybe we could just expand the behaviour of this option by opening it up to strings/enums and other types?

var defaultValue: String?
if options.entities.includeDefaultValues {
if type.isBool {
defaultValue = (info?.defaultValue?.value as? Bool).map { $0 ? "true" : "false" }
}
}

This flag relies on the schema to have defined the default value though, so it might be a bit different to your suggestion where we should infer a default value automatically... I'm just thinking about how we'd implement it in a way that isn't too confusing with both behaviours side by side

@aaronsky
Copy link
Author

aaronsky commented Nov 23, 2022

The ASC schema does not specify a default in the spec itself, unfortunately. It would've made sense if they had though. My thinking was, considering these values must be set and can canonically only ever have one possible value, it would be practical to follow that logic to its inevitable conclusion and save users the trouble. I will defer to you on how you want this implemented.

@liamnichols
Copy link
Member

I haven't thought about it too much, but I think it seems like it would be acceptable for includeDefaultValues to do the following:

  1. If a default value is set on the schema, include it (for enum/string and maybe other primitive types too).
  2. If a default value isn't set on the schema, and there can only be one possible value, use that.

As long as we describe that in the documentation, I think thats a good approach. How does it sound to you? Would you be up for implementing default values from the schema for string, integer and number types too as part of this? I think it would be a good improvement that shouldn't be too complicated (at least I don't think it should 😅)

@aaronsky
Copy link
Author

That sounds like a reasonable approach. I'll let you know if I have any more questions!

@aaronsky
Copy link
Author

In your opinion, if a default value is provided or derived through your approach, how should that affect the optionality of the property? Looking at the edgecases example:

public struct Animal: Codable {
    ...
    public var color: String?
    
    public init(... color: String? = nil) {

We provide a default value of "red" in the spec. Another way we could write this is:

public struct Animal: Codable {
    ...
    public var color: String
    
    public init(... color: String = "red") {

...but this comes with a different meaning. Should isOptional properties always be nullable if they are optional (not explicitly required in the spec), or should they be affected by the presence of a default?

@liamnichols
Copy link
Member

...but this comes with a different meaning. Should isOptional properties always be nullable if they are optional (not explicitly required in the spec), or should they be affected by the presence of a default?

If I understand correctly think nullable/optional properties will need to retain their presence because the schema/entity could be used in both requests and responses, and while a default might be used when initialising, the response might need to successfully decode null into an optional still.

So with a that, I'd imagine the generated code for Animal would look like this:

public struct Animal: Codable {
    ...
    public var color: String?
    
    public init(... color: String? = "red") {

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 a pull request may close this issue.

2 participants