NotificationsYou must be signed in to change notification settings
Fork2.2k
Star24.6k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Viicos merged 2 commits intomainfromdefinitions-improvements

Jan 7, 2025

Merged

Consolidate schema definitions logic in the`_Definitions` class#11208

Viicos merged 2 commits intomainfromdefinitions-improvements

Jan 7, 2025

Conversation

Copy link

Member

Viicos commentedJan 2, 2025

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

The pull request title is a good summary of the changes - it will be used in the changelog
Unit tests for the changes exist
Tests pass on CI
Documentation reflects the changes where applicable
My PR is ready to review,please add a comment including the phrase "please review" to assign reviewers

github-actionsbot added the relnotes-fixUsed for bugfixes. label

Jan 2, 2025

Viicos force-pushed thedefinitions-improvements branch fromfcd4818 to5de851fCompare

January 2, 2025 22:17

Copy link

cloudflare-workers-and-pagesbot commentedJan 2, 2025•
edited
Loading

Deploying pydantic-docs with Cloudflare Pages

Latest commit:	`c388c19`
Status:	✅ Deploy successful!
Preview URL:	https://506c3228.pydantic-docs.pages.dev
Branch Preview URL:	https://definitions-improvements.pydantic-docs.pages.dev

View logs

Viicos commented

Jan 2, 2025

View reviewed changes

pydantic/_internal/_generate_schema.py

		self.seen: set[str] = set()
		self.definitions: dict[str, core_schema.CoreSchema] = {}
		self._recursively_seen = set()
		self._definitions = {}

Copy link

MemberAuthor

ViicosJan 2, 2025

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 fromcreate_reference_to_schema and definitions coming fromunpack_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?

Copy link

codspeed-hqbot commentedJan 2, 2025•
edited
Loading

CodSpeed Performance Report

Merging#11208 willnot alter performance

_{Comparingdefinitions-improvements (c388c19) withmain (da0449e)}

Summary

✅ 46 untouched benchmarks

Viicos commented

Jan 2, 2025

View reviewed changes

pydantic/_internal/_generate_schema.py Outdated

Comment on lines 827 to 829

		# if schema['type'] == 'definition-ref':
		# return core_schema.definition_reference_schema(schema_ref=schema['schema_ref'])

Copy link

MemberAuthor

ViicosJan 2, 2025

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?

Copy link

MemberAuthor

ViicosJan 7, 2025

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.

Copy link

Contributor

github-actionsbot commentedJan 2, 2025•
edited
Loading

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic/_internal
_generate_schema.py					2121-2123
_schema_generation_shared.py
Project Total

_{This report was generated bypython-coverage-comment-action}

Viicos force-pushed thedefinitions-improvements branch from5de851f toe3a19ebCompare

January 7, 2025 14:59

Viicos marked this pull request as ready for review

January 7, 2025 14:59

Viicos force-pushed thedefinitions-improvements branch frome3a19eb tob6e5c54Compare

January 7, 2025 15:01

sydney-runkle reviewed

Jan 7, 2025

View reviewed changes

Copy link

Contributor

sydney-runkle left a 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.

pydantic/_internal/_generate_schema.py OutdatedShow resolvedHide resolved

Consolidate schema definitions logic in the_Definitions class

79bec86

Some common patterns are defined as methods to avoid duplication(e.g. `create_reference_to_schema`, used to store a schema as adefinition and returns a `'definition-reference'` schema). A coupleexisting methods on the `GenerateSchema` class are moved (andrenamed to better describe what they do) to the `_Definitions`class.Proper docstrings are added to attribute and methods.Type hints are tweaked when necessary.

Viicos force-pushed thedefinitions-improvements branch fromb6e5c54 to79bec86Compare

January 7, 2025 15:35

Remove schema copy

c388c19

sydney-runkle approved these changes

Jan 7, 2025

View reviewed changes

Copy link

Contributor