Skip to content

Conversation

sydney-runkle
Copy link
Contributor

@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-workers-and-pages
Copy link

cloudflare-workers-and-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
Contributor Author

Please review

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

@sydney-runkle sydney-runkle added awaiting author revision awaiting changes from the PR author and removed ready for review labels Sep 29, 2023
sydney-runkle and others added 2 commits September 29, 2023 10:55
Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>
@sydney-runkle sydney-runkle added ready for review and removed awaiting author revision awaiting changes from the PR author labels Sep 29, 2023
@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
@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