-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add testing coverage for pretty_print #9469
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
Add testing coverage for pretty_print #9469
Conversation
CodSpeed Performance ReportMerging #9469 will not alter performanceComparing Summary
|
|
please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for the contribution! I'm guessing some of the linting tests might fail, but you should be able to run make format and make lint locally to fix those issues!
|
Hmph, looks like there are still some errors (tests failing). Once that's resolved, all looks good! |
tests/test_utils.py
Outdated
| metadata={'schema_type': 'any', 'test_id': '42'}, | ||
| serialization=core_schema.simple_ser_schema('bool'), | ||
| ) | ||
| expected_meta_info = "{\n 'type': 'any',\n 'ref': 'meta_schema',\n 'metadata': {'schema_type': 'any', 'test_id': '42'},\n 'serialization': {'type': 'bool'}\n}\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe multiline strings could be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Viicos, yep, I think we're just waiting on formatting changes, but otherwise looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions! I'll go through and get the tests passing in the workflow later today and then re-request review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes made and checks passed @sydney-runkle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, we're glad to have a test for this!
Change Summary
Added test coverage to CoreSchema pretty print utility function
Related issue number
#7656
Checklist
Selected Reviewer: @dmontagu