Skip to content

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented Jul 17, 2023

Closes #6663

Selected Reviewer: @lig

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 17, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1380b8c
Status: ✅  Deploy successful!
Preview URL: https://1a721c94.pydantic-docs2.pages.dev
Branch Preview URL: https://json-content-schema.pydantic-docs2.pages.dev

View logs

@dmontagu
Copy link
Contributor Author

please review

Copy link
Contributor

@lig lig left a comment

Choose a reason for hiding this comment

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

👍🏻

@@ -1609,13 +1609,13 @@ def json_schema(self, schema: core_schema.JsonSchema) -> JsonSchemaValue:
Returns:
The generated JSON schema.
"""
content_core_schema = schema['schema'] if 'schema' in schema else core_schema.any_schema()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like the following code more but it will always call core_schema.any_schema()

Suggested change
content_core_schema = schema['schema'] if 'schema' in schema else core_schema.any_schema()
content_core_schema = schema.get('schema', core_schema.any_schema())

It would be nice to have a constant like core_schema.DEFAULT_SCHEMA that will hold this value and allow the above to be as efficient as if..in.

@@ -1609,13 +1609,13 @@ def json_schema(self, schema: core_schema.JsonSchema) -> JsonSchemaValue:
Returns:
The generated JSON schema.
"""
content_core_schema = schema['schema'] if 'schema' in schema else core_schema.any_schema()
Copy link
Contributor Author

@dmontagu dmontagu Jul 19, 2023

Choose a reason for hiding this comment

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

Suggested change
content_core_schema = schema['schema'] if 'schema' in schema else core_schema.any_schema()
content_core_schema = schema.get('schema') or core_schema.any_schema()

I think this is the zero-overhead version of your suggestion, which works since all schemas must have a type key so schema.get('schema') will be falsy only if it is None (and having the value None is not meaningfully different from not being present)

@dmontagu dmontagu enabled auto-merge (squash) July 19, 2023 21:05
@dmontagu dmontagu merged commit 8c212f9 into main Jul 19, 2023
@dmontagu dmontagu deleted the json-content-schema branch July 19, 2023 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Json[type] schema generated now returns string rather than type
2 participants