Skip to content

Conversation

Viicos
Copy link
Member

@Viicos Viicos commented Aug 27, 2024

This happens when MotelMetaclass is used as a metaclass for a class other than BaseModel.

Fixes #10239

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

This happens when `MotelMetaclass` is used as a metaclass
for a class other than `BaseModel`.
@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Aug 27, 2024
Copy link

cloudflare-workers-and-pages bot commented Aug 27, 2024

Deploying pydantic-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 79ffa7c
Status: ✅  Deploy successful!
Preview URL: https://d1d89a9d.pydantic-docs.pages.dev
Branch Preview URL: https://metaclass-fix.pydantic-docs.pages.dev

View logs

Copy link

codspeed-hq bot commented Aug 27, 2024

CodSpeed Performance Report

Merging #10242 will not alter performance

Comparing metaclass-fix (79ffa7c) with main (ed8df66)

Summary

✅ 17 untouched benchmarks

Copy link
Contributor

github-actions bot commented Aug 27, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Can you add a test, even if it's weird and specific.

@@ -243,7 +243,8 @@ def wrapped_model_post_init(self: BaseModel, context: Any, /) -> None:
else:
# These are instance variables, but have been assigned to `NoInitField` to trick the type checker.
for instance_slot in '__pydantic_fields_set__', '__pydantic_extra__', '__pydantic_private__':
del namespace[instance_slot]
if instance_slot in namespace: # Safe check in case the metaclass is used other than in `BaseModel`:
del namespace[instance_slot]
Copy link
Member

Choose a reason for hiding this comment

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

Surely, nameapace.pop(instance_slot, None) is better - only one lookup of the dict.

Copy link
Contributor

@sydney-runkle 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.

Thanks for the quick fix here!

In the future, let's break PRs like #10110 up into two - simple annotation order switches to adhere to type checker rules, and separately, actual code changes.

@Viicos
Copy link
Member Author

Viicos commented Aug 27, 2024

In the future, let's break PRs like #10110 up into two - simple annotation order switches to adhere to type checker rules, and separately, actual code changes.

Well in this case the two were required to work together.

@sydney-runkle
Copy link
Contributor

Ah I see, fair enough. And the diff was small enough on the PR that things weren't too difficult to track down.

@sydney-runkle sydney-runkle enabled auto-merge (squash) August 27, 2024 13:05
@Viicos Viicos disabled auto-merge August 27, 2024 13:11
@Viicos Viicos enabled auto-merge (squash) August 27, 2024 13:11
@Viicos Viicos merged commit 9ad349a into main Aug 27, 2024
59 checks passed
@Viicos Viicos deleted the metaclass-fix branch August 27, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes-fix Used for bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v2.9.0b1 is breaking napari
3 participants