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
Simplify handling of typing.Annotated in GenerateSchema #6887
Conversation
@@ -391,6 +392,17 @@ def collect_definitions(self, schema: CoreSchema) -> CoreSchema: | |||
list(self.defs.definitions.values()), | |||
) | |||
|
|||
def _add_js_function(self, metadata_schema: CoreSchema, js_function: Callable[..., Any]) -> None: |
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.
this function just moved, no changes were made
Deploying with Cloudflare Pages
|
if js_function not in pydantic_js_functions: | ||
pydantic_js_functions.append(js_function) | ||
|
||
def _generate_schema_for_type( |
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.
I eliminated this method since the only place it was called was in generate_schema
, and with the removal of the handling of _annotated_schema
there, generate_schema
was just immediately calling this function with the exact same signature and arguments passing through without modification.
if isinstance(obj, type(Annotated[int, 123])): | ||
schema = transform_inner_schema(self._annotated_schema(obj)) |
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.
This branch no longer seemed necessary now that _generate_schema
handles Annotated
properly (and Annotated
doesn't hit any of the other property extractions as far as I can tell). So I just eliminated the branch here, that's all that's happening in this chunk.
@@ -87,6 +87,7 @@ | |||
from ._schema_generation_shared import GetJsonSchemaFunction | |||
|
|||
_SUPPORTS_TYPEDDICT = sys.version_info >= (3, 12) | |||
_AnnotatedType = type(Annotated[int, 123]) |
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.
Repeated calls to type
and Annotated.__class_getitem__
while building schemas seemed unnecessary/inefficient, but I didn't profile this or anything. Probably negligible overhead but just ... rubbed me the wrong way reading isinstance(obj, type(Annotated[int, 123]))
in a frequently-executed loop (at least during model creation) lol. Can revert this change if preferred.
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 good to me!
Closes #6884
@adriangb It seems to me some of the cruft removed in this PR might have been left around from previous fiddling that has been rendered unnecessary. But if you see issues with this we can revert some of the other changes; I'll note that an alternative diff that also closes #6884 is just:
But looking into the code, it seemed to me this was a "safer" solution. But I think you know this code best at this point.