Skip to content
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

Add support for instance method reassignment when extra='allow' #7683

Merged
merged 7 commits into from Oct 2, 2023

Conversation

sydney-runkle
Copy link
Member

@sydney-runkle sydney-runkle commented Sep 28, 2023

Change Summary

Adding aligning the logic of __setattr__ and __getattr__ when extra=True on a model's config.

Related issue number

Fix #7631

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

Selected Reviewer: @Kludex

@sydney-runkle sydney-runkle added the relnotes-change Used for changes to existing functionality which don't have a better categorization. label Sep 28, 2023
@cloudflare-pages
Copy link

cloudflare-pages bot commented Sep 28, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 46db5a7
Status: ✅  Deploy successful!
Preview URL: https://52b63d03.pydantic-docs2.pages.dev
Branch Preview URL: https://fix-setattr-with-extra-setti.pydantic-docs2.pages.dev

View logs

@sydney-runkle
Copy link
Member Author

Please review

Reviews welcome - not super set on a given pattern / if we should even do this, just looking for feedback 😄

pydantic/main.py Outdated Show resolved Hide resolved
tests/test_main.py Outdated Show resolved Hide resolved
sydney-runkle and others added 2 commits September 29, 2023 10:55
Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>
@adriangb
Copy link
Member

What happens if the model is also frozen?

@dmontagu
Copy link
Contributor

Lol good question.

I checked, and:

  • On main, you get an error if you try to replace a method on a frozen model instance with or without extra='allow'.
  • On this branch, you get an error if you try to replace a method on a frozen model instance with or without extra='allow'.
  • For what it's worth, dataclasses raises an error if you try to overwrite a method on a frozen dataclass instance

I think this is reasonable behavior for frozen models (i.e., frozen instances don't let you overwrite any attributes, not just fields/extra/etc.). And since it raises an error, if we did ever want to change this in the future it would be easier to justify in terms of backwards compatibility than going in the other direction (i.e., making a non-error into an error).

@sydney-runkle sydney-runkle merged commit 8bf4aa0 into main Oct 2, 2023
62 checks passed
@sydney-runkle sydney-runkle deleted the fix-setattr-with-extra-setting branch October 2, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review relnotes-change Used for changes to existing functionality which don't have a better categorization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setattribute with Extra.allow is hiding method when it's defined on class.
4 participants