Skip to content

Fix boolean json schemas#515

Merged
bcherny merged 6 commits intobcherny:masterfrom
notaphplover:fix/boolean-json-schemas
May 4, 2023
Merged

Fix boolean json schemas#515
bcherny merged 6 commits intobcherny:masterfrom
notaphplover:fix/boolean-json-schemas

Conversation

@notaphplover
Copy link
Copy Markdown
Contributor

Added

  • Added NEVER schema type.

Changed

  • Updated parser to correctly parse boolean JSON schemas.

@notaphplover notaphplover marked this pull request as ready for review February 23, 2023 23:07
@notaphplover
Copy link
Copy Markdown
Contributor Author

Hey @bcherny, this should do the trick to support boolean JSON schemas. Do you have any test cases in mind?

I added a couple of cases but I'm open to add / remove more :)

Comment thread src/parser.ts
return parseBooleanSchema(schema, keyName, options)
}

return parseLiteral(schema, keyName)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

tbh I don't know what to do here. In theory we should omit anything at this point instead of parsing the literal, but I don't want to introduce too many changes in this PR

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could be a little cleaner to move the parseBooleanSchema into parseLiteral, since true/false are literals.

Copy link
Copy Markdown
Contributor Author

@notaphplover notaphplover Apr 26, 2023

Choose a reason for hiding this comment

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

I think this would be a mistake. Parsing true and false as schemas is not the same as parsing them as values. Depending on the context, true and false are schemas or values.

A simple example would be the true boolean schema vs a true enum value. They should not be parsed as the same.

Comment thread src/types/JSONSchema.ts
Copy link
Copy Markdown
Owner

@bcherny bcherny left a comment

Choose a reason for hiding this comment

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

This looks great -- thanks for the contribution! Couple of minor suggestions. Could you also please add test cases for top-level true/false schemas?

ie.

export const input = true
export const input = false

Comment thread src/parser.ts Outdated
Comment thread src/parser.ts
return parseBooleanSchema(schema, keyName, options)
}

return parseLiteral(schema, keyName)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could be a little cleaner to move the parseBooleanSchema into parseLiteral, since true/false are literals.

@notaphplover
Copy link
Copy Markdown
Contributor Author

notaphplover commented May 2, 2023

Hey @bcherny, when adding the root boolean schema case, it seems the ref parser can not dereference it, it throws an error instead. I can have a look at it, but seems the parser is expecting an object instead of a boolean value.

Would it be ok if I submit a pr in the parser repo? I think some sort of conditional logic to handle boolean schemas would do the trick.

Copy link
Copy Markdown
Owner

@bcherny bcherny left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the contribution!

@bcherny bcherny merged commit 05b0103 into bcherny:master May 4, 2023
@bcherny
Copy link
Copy Markdown
Owner

bcherny commented May 4, 2023

Published 13.0.0.

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