-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Consolidate schema definitions logic in the _Definitions
class
#11208
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
Conversation
fcd4818
to
5de851f
Compare
Deploying pydantic-docs with
|
Latest commit: |
c388c19
|
Status: | ✅ Deploy successful! |
Preview URL: | https://506c3228.pydantic-docs.pages.dev |
Branch Preview URL: | https://definitions-improvements.pydantic-docs.pages.dev |
self.seen: set[str] = set() | ||
self.definitions: dict[str, core_schema.CoreSchema] = {} | ||
self._recursively_seen = set() | ||
self._definitions = {} |
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.
@MarkusSintonen, in your PR, you made a distinction between definitions stored from create_reference_to_schema
and definitions coming from unpack_definitions
(in your PR, as _unpacked_definitions
), i.e. definitions potentially coming from the cached attribute of Pydantic models.
What was the reason to do so?
CodSpeed Performance ReportMerging #11208 will not alter performanceComparing Summary
|
# if schema['type'] == 'definition-ref': | ||
# return core_schema.definition_reference_schema(schema_ref=schema['schema_ref']) |
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.
@MarkusSintonen, this is an addition in your PR. Seems like test passes without it. Do you remember why this might be required? I'm assuming this is part of #10655, where 'definition-ref'
schemas are inlined in place, and thus this might some kind of safety copy?
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'm removing it from this PR, as if it needs to be included, this will be in the schema walking refactor PR.
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
5de851f
to
e3a19eb
Compare
e3a19eb
to
b6e5c54
Compare
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 like this refactor overall, I think it does simplify things and make our already confusing defs/refs management logic a bit more clear.
Thanks for adding those docstrings as well.
Some common patterns are defined as methods to avoid duplication (e.g. `create_reference_to_schema`, used to store a schema as a definition and returns a `'definition-reference'` schema). A couple existing methods on the `GenerateSchema` class are moved (and renamed to better describe what they do) to the `_Definitions` class. Proper docstrings are added to attribute and methods. Type hints are tweaked when necessary.
b6e5c54
to
79bec86
Compare
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'm happy with this given the re-namings, thanks!
Some common patterns are defined as methods to avoid duplication (e.g.
create_reference_to_schema
, used to store a schema as a definition and returns a'definition-reference'
schema). A couple existing methods on theGenerateSchema
class are moved (and renamed to better describe what they do) to the_Definitions
class.Proper docstrings are added to attribute and methods.
Type hints are tweaked when necessary.
This is meant to be a simplified changeset of #10887, only with the necessary changes present.
Change Summary
Related issue number
Checklist