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

Recommend against using`Ellipsis` (`...`) with`Field`#10661

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 1 commit intomainfromfield-ellipsis

Oct 21, 2024

Merged

Recommend against using`Ellipsis` (`...`) with`Field`#10661

Viicos merged 1 commit intomainfromfield-ellipsis

Oct 21, 2024

Conversation

Copy link

Member

Viicos commentedOct 18, 2024

Also fix a bug where under certain scenarios, aField function insideAnnotated metadata would retain theEllipsis as a default value.

Also makePrivateAttr on par withField and the special casing ofEllipsis.

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

Oct 18, 2024

Viicos commented

Oct 18, 2024

View reviewed changes

docs/concepts/models.md

		@@ -1359,25 +1359,23 @@ print(error_locations)

		## Required fields

Copy link

MemberAuthor

ViicosOct 18, 2024

Choose a reason for hiding this comment

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

This is the updated documentation, the rest is just switching fromField(...) toField()

Copy link

cloudflare-workers-and-pagesbot commentedOct 18, 2024•
edited
Loading

Deploying pydantic-docs with Cloudflare Pages

Latest commit:	`76cc978`
Status:	✅ Deploy successful!
Preview URL:	https://8f7cad7a.pydantic-docs.pages.dev
Branch Preview URL:	https://field-ellipsis.pydantic-docs.pages.dev

View logs

Viicos commented

Oct 18, 2024

View reviewed changes

pydantic/fields.py

Comment on lines +212 to +215

		# Also remove it from the attributes set, otherwise
		# `GenerateSchema._common_field_schema` mistakenly
		# uses it:
		self._attributes_set.pop('default', None)

Copy link

MemberAuthor

ViicosOct 18, 2024

Choose a reason for hiding this comment

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

See the added test for an example

Viicos commented

Oct 18, 2024

View reviewed changes

tests/test_discriminated_union.py

		adapter = TypeAdapter(AnyState)

		assert adapter.core_schema['schema']['type'] == 'tagged-union'
		for definition in adapter.core_schema['definitions']:
		if definition['schema']['model_name'] in ['NestedState', 'LoopState']:
		assert definition['schema']['fields']['substate']['schema']['schema']['type'] == 'tagged-union'
		assert definition['schema']['fields']['substate']['schema']['type'] == 'tagged-union'

Copy link

MemberAuthor

ViicosOct 18, 2024•
edited
Loading

Choose a reason for hiding this comment

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

As a consequence of the fixed bug: a slight change in schemas becauseAnyState is no longer considered to have a default value of....

Copy link

Contributor

sydney-runkleOct 18, 2024

Choose a reason for hiding this comment

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

Thanks for explaining. I'm surprised this changed the schema - I guess there was awith_default schema here previously?

Copy link

MemberAuthor

ViicosOct 18, 2024

Choose a reason for hiding this comment

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

yes exactly

Copy link

Contributor

sydney-runkleOct 18, 2024

Choose a reason for hiding this comment

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

Seems like a bug fix.

Copy link

codspeed-hqbot commentedOct 18, 2024•
edited
Loading

CodSpeed Performance Report

Merging#10661 willnot alter performance

_{Comparingfield-ellipsis (76cc978) withmain (220b2f6)}

Summary

✅ 44 untouched benchmarks

Viicos force-pushed thefield-ellipsis branch from8a0ce6d toa1d8176Compare

October 18, 2024 18:16

Copy link

Contributor

github-actionsbot commentedOct 18, 2024

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic
fields.py
Project Total

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

sydney-runkle approved these changes

Oct 18, 2024

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.

Nice. One quick question, but looks great.

Surprised re how many places we used this in the docs / examples. Nice job with the update!

docs/concepts/models.md OutdatedShow resolvedHide resolved

tests/test_discriminated_union.py

		adapter = TypeAdapter(AnyState)

		assert adapter.core_schema['schema']['type'] == 'tagged-union'
		for definition in adapter.core_schema['definitions']:
		if definition['schema']['model_name'] in ['NestedState', 'LoopState']:
		assert definition['schema']['fields']['substate']['schema']['schema']['type'] == 'tagged-union'
		assert definition['schema']['fields']['substate']['schema']['type'] == 'tagged-union'

Copy link

Contributor

sydney-runkleOct 18, 2024

Choose a reason for hiding this comment

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

Thanks for explaining. I'm surprised this changed the schema - I guess there was awith_default schema here previously?

Recommend against usingEllipsis (...) withField

76cc978

Also fix a bug where under certain scenarios, a `Field` functioninside `Annotated` metadata would retain the `Ellipsis` as adefault value.Also make `PrivateAttr` on par with `Field` and the special casingof `Ellipsis`.

Viicos force-pushed thefield-ellipsis branch froma1d8176 to76cc978Compare

October 21, 2024 08:28

Viicos merged commita1ffc82 intomain

Oct 21, 2024

62 checks passed

Viicos deleted the field-ellipsis branch

October 21, 2024 08:40

Viicos mentioned this pull request

Nov 4, 2024

Required with Ellipsis may not workfastapi/fastapi#12313

Closed

1 task

sydney-runkle mentioned this pull request

Nov 6, 2024

2.10 breaks type checking forField(...)#10780

Closed

1 task

Labels

relnotes-fix

Used for bugfixes.

2 participants

Movatterモバイル変換

Uh oh!

Recommend against usingEllipsis (...) withField#10661

Recommend against usingEllipsis (...) withField#10661

Uh oh!

Conversation

Viicos commentedOct 18, 2024

Change Summary

Related issue number

Checklist

Uh oh!

ViicosOct 18, 2024

Choose a reason for hiding this comment

Uh oh!

cloudflare-workers-and-pagesbot commentedOct 18, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Deploying pydantic-docs with Cloudflare Pages

Uh oh!

ViicosOct 18, 2024

Choose a reason for hiding this comment

Uh oh!

ViicosOct 18, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sydney-runkleOct 18, 2024

Choose a reason for hiding this comment

Uh oh!

ViicosOct 18, 2024

Choose a reason for hiding this comment

Uh oh!

sydney-runkleOct 18, 2024

Choose a reason for hiding this comment

Uh oh!

codspeed-hqbot commentedOct 18, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging#10661 willnot alter performance

Summary

Uh oh!

github-actionsbot commentedOct 18, 2024

Coverage report

Uh oh!

sydney-runkle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sydney-runkleOct 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Recommend against using`Ellipsis` (`...`) with`Field`#10661

Recommend against using`Ellipsis` (`...`) with`Field`#10661

cloudflare-workers-and-pagesbot commentedOct 18, 2024•
edited
Loading

ViicosOct 18, 2024•
edited
Loading

codspeed-hqbot commentedOct 18, 2024•
edited
Loading