Skip to content

Conversation

donny-dont
Copy link

Prefer using nullable: true over oneOf: [foo, null] and type: [foo, null].

The nullable: true value should only be used if the property is contained in required so add the property to that where applicable.

Prefer using `nullable: true` over `oneOf: [foo, null]` and `type: [foo, null]`.

The `nullable: true` value should only be used if the property is contained in `required` so add the property to that where applicable.
Copy link
Author

@donny-dont donny-dont left a comment

Choose a reason for hiding this comment

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

Here's the spec on what nullable is and where it should be used https://swagger.io/docs/specification/v3_0/data-models/data-types/#null

Just had a couple questions about correctness within the actual output of the API.

Comment on lines +3182 to +3183
$ref: "#/components/schemas/PaintOverride"
nullable: true
Copy link
Author

Choose a reason for hiding this comment

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

Is the value associated with a key in fillOverrideTable ever null? Seems like you just wouldn't have the key in the object.

Comment on lines +5247 to +5248
type: string
nullable: true
Copy link
Author

Choose a reason for hiding this comment

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

Added this here but it shouldn't be needed since the field isn't in required.

Wondering if BaseTypeStyle does have some required fields that aren't noted in this definition.

Comment on lines +8699 to +8700
type: string
nullable: true
Copy link
Author

Choose a reason for hiding this comment

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

Same as ☝️ in fillOverrideTable where its not clear if the images object will ever have a value where object.value == null.

Copy link

@monica-goren-figma monica-goren-figma left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ahurtado-figma ahurtado-figma left a comment

Choose a reason for hiding this comment

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

Holding this PR to give it a bit more review -- thank you for your patience so far @donny-dont 🙏🏼

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.

3 participants