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: schema generation can fail #875

Merged
merged 3 commits into from
Dec 2, 2019
Merged

Fix: schema generation can fail #875

merged 3 commits into from
Dec 2, 2019

Conversation

XVincentX
Copy link
Contributor

@XVincentX XVincentX commented Dec 2, 2019

While debugging some JSON Ref issues I noticed that Schema Generation can effectively throw exceptions and would be treated that well.

This PR correctly wraps such situations in tryCatch statements.

@XVincentX XVincentX force-pushed the fix/schema-gen-crash branch 3 times, most recently from 236b91c to df12b84 Compare December 2, 2019 12:31
@XVincentX XVincentX marked this pull request as ready for review December 2, 2019 12:31
Copy link
Contributor

@karol-maciaszek karol-maciaszek left a comment

Choose a reason for hiding this comment

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

Please add missing test case while I'm taking time to learn what Record is

const instance = generate(schema);
expect(instance).toHaveProperty('ip');
const ip = get(instance, 'ip');
assertRight(generate(schema), instance => {
Copy link
Contributor

@karol-maciaszek karol-maciaszek Dec 2, 2019

Choose a reason for hiding this comment

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

Since generate returns Either we should have a test of a failure. Maybe a circular object will do the job?

Copy link
Contributor

Choose a reason for hiding this comment

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

ekhm :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahi, I didn't see this. Will add.

Copy link
Contributor

@karol-maciaszek karol-maciaszek left a comment

Choose a reason for hiding this comment

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

I'm tentatively accepting since I'm going home, I learned what Record is and it looks way cooler now.

@XVincentX XVincentX merged commit f9c3412 into master Dec 2, 2019
@XVincentX XVincentX deleted the fix/schema-gen-crash branch December 2, 2019 17:06
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.

None yet

2 participants