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

fix: we are sure about type if parent checked it #177

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented Jan 25, 2024

Type delta calculation here is needed for discriminator to be sure that we can compile.

Fixes: #176.

This is just the first part of the fix that will work if type: object is specified on levels 1 and 2 or level 3.

An additional fix is needed to allow specifying type: object only once on level 1.

See also explanation in #176 (comment)

Type delta calculation here is needed for discriminator to be sure
that we can compile.
@mvayngrib
Copy link

mvayngrib commented Jan 27, 2024

is there a breaking unit test we can add for this case? (a part of the test case Konnor added in the issue?)

ChALkeR added a commit that referenced this pull request Jan 27, 2024
@ChALkeR
Copy link
Contributor Author

ChALkeR commented Jan 27, 2024

@mvayngrib done. #179 will improve it once this lands.

Upd: I'll update the testcase.
Upd: Done, #179 will need just one line removed for extra tests.

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b549d02) 99.54% compared to head (c87a7cc) 99.54%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
Files Coverage Δ
src/compile.js 99.29% <100.00%> (+<0.01%) ⬆️

ChALkeR added a commit that referenced this pull request Jan 27, 2024
@ChALkeR ChALkeR force-pushed the chalker/fix-discriminator-typecheck/0 branch from 731e2c6 to 75dd90b Compare January 27, 2024 15:40
mvayngrib
mvayngrib previously approved these changes Jan 29, 2024
test/regressions/177.js Show resolved Hide resolved
@ChALkeR
Copy link
Contributor Author

ChALkeR commented Jan 29, 2024

This needs a second review per the new process.

@mvayngrib mvayngrib requested a review from kklash January 29, 2024 20:04
@mvayngrib
Copy link

@kklash one more time please 😅. We're trying out a more strict review process. I'm already not a fan

@mvayngrib
Copy link

@ChALkeR this good to go?

@ChALkeR ChALkeR merged commit 8adc130 into master Jan 31, 2024
10 checks passed
@ChALkeR ChALkeR deleted the chalker/fix-discriminator-typecheck/0 branch January 31, 2024 17:35
@ChALkeR ChALkeR mentioned this pull request Feb 1, 2024
1 task
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.

bug: discriminator compilation fails for discriminators nested in oneOf
4 participants