Skip to content

Fix oneOf number const #1056

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

Merged
merged 3 commits into from
Mar 30, 2023
Merged

Conversation

qnp
Copy link
Contributor

@qnp qnp commented Mar 23, 2023

Changes

Fixing #1051 alongside the fact that 0 as a const value was evicted.

How to Review

I wrote tests, they all pass now.

FYI, I wanted to make sure everything is working as expected, so I added tests for the tsOneOf util. IMHO, all the utils should be tested (but I did not have time to write these tests).

Checklist

  • Unit tests updated
  • README updated => Not relevant for the bugfix
  • examples/ directory updated (if applicable)

@@ -98,7 +98,7 @@ export function defaultSchemaObjectTransform(
// oneOf (no discriminator)
if ("oneOf" in schemaObject && !schemaObject.oneOf.some((t) => "$ref" in t && ctx.discriminators[t.$ref])) {
const maybeTypes = schemaObject.oneOf.map((item) => transformSchemaObject(item, { path, ctx }));
if (maybeTypes.some((t) => t.includes("{"))) return tsOneOf(...maybeTypes); // OneOf<> helper needed if any objects present ("{")
if (maybeTypes.some((t) => typeof t === "string" && t.includes("{"))) return tsOneOf(...maybeTypes); // OneOf<> helper needed if any objects present ("{")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -62,7 +62,7 @@ export function defaultSchemaObjectTransform(
}

// const (valid for any type)
if (schemaObject.const) {
if (schemaObject.const !== null && schemaObject.const !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,21 @@
import { tsUnionOf } from "../src/utils";

describe("utils", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests aren’t really necessary as they are all tested by proxy with other tests, and these are such simple functions with such simple inputs/outputs they have clear and obvious failures.

However, the coverage is appreciated and it doesn’t hurt to have. Thank you.

Copy link
Contributor

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

Great fix; thank you!

@drwpow
Copy link
Contributor

drwpow commented Mar 30, 2023

@all-contributors please add @qnp for code, bug, test

@allcontributors
Copy link
Contributor

@drwpow

I've put up a pull request to add @qnp! 🎉

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