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

Add a hint for allowed values types #246

Merged
merged 3 commits into from
Dec 22, 2021

Conversation

kean
Copy link
Contributor

@kean kean commented Dec 21, 2021

Cherry-pick of #245 🍒

@kean
Copy link
Contributor Author

kean commented Dec 21, 2021

Hmm, I found a potential caveat with approach:

Nullable enum
A nullable enum can be defined as follows:

type: string
nullable: true  # <---
enum:
  - asc
  - desc
  - null        # <--- without quotes, i.e. null not "null"

Note that null must be explicitly included in the list of enum values. Using nullable: true alone is not enough here.

From https://swagger.io/docs/specification/data-models/enums/

Unbelievable. I don't understand why. Also null and "null" are different things. I'm also wondering how this is going to work in JSON?? You can't do "["a", null]", as far as I know.

I have this exact issue in one of the specs I'm testing against (Soundcloud):

access:
    type: string
    nullable: true
    description: |
    Level of access the user (logged in or anonymous) has to the track.
        * `playable` - user is allowed to listen to a full track.
        * `preview` - user is allowed to preview a track, meaning a snippet is available
        * `blocked` - user can only see the metadata of a track, no streaming is possible
    enum:
    - playable
    - preview
    - blocked
    - null

@kean
Copy link
Contributor Author

kean commented Dec 21, 2021

As far as I can tell, almost nobody adds "null" to their enums and just use nullable. I've seen thousands of examples of proper optional enums without null. But ideally it should be addressed somehow. Maybe StringOrNull decodable type?

@mattpolzin
Copy link
Owner

Good catch. I think this is what we get if we change it to [String?] but it's been a while so I'd have to try it out and see.

@mattpolzin
Copy link
Owner

mattpolzin commented Dec 22, 2021

This should take care of the problem you uncovered: #247

@mattpolzin
Copy link
Owner

You can't do "["a", null]", as far as I know.

Since in JavaScript everything is nullable, this is fine.

Copy link
Owner

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting (again) and catching the null string issue so fast!

@mattpolzin mattpolzin merged commit 6316f4f into mattpolzin:main Dec 22, 2021
@kean kean deleted the kean/properties-hint-2 branch December 23, 2021 00:49
@kean
Copy link
Contributor Author

kean commented Dec 23, 2021

Thanks! I updated to the latest OpenAPIKit version and it now works as expected.

I also just made my tool public - https://github.com/kean/CreateAPI. I hope to release it soon too.

@mattpolzin
Copy link
Owner

I also just made my tool public

Congrats! I haven't dug in more than the README yet, but that's a powerful sales pitch. Very exciting project!

You absolutely don't need to convince me of anything, but how would you compare your goals for CreateAPI to the execution of the SwagGen project as of now?

@kean
Copy link
Contributor Author

kean commented Dec 23, 2021

The main difference is execution. I've tried multiple code generators, not just SwagGen. They all produced code that doesn't compile. I tested with the GitHub spec as an input - that's the primary spec I'm optimizing for. So that's already not hard to beat. But I go way beyond just making sure the code compiles.

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.

2 participants