Navigation Menu

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

Fix PEP487 __set_name__ protocol in BaseModel #4407

Merged
merged 7 commits into from Aug 21, 2022

Conversation

tlambert03
Copy link
Contributor

@tlambert03 tlambert03 commented Aug 20, 2022

Change Summary

This is a small change that brings BaseModel back into compliance with the the __set_name__ protocol defined in [PEP 487, and described in the creating the class object portion of the python documentation, which states:

the following additional customization steps are invoked after creating the class object:

  • The type.__new__ method collects all of the attributes in the class namespace that define a __set_name__() method;
  • Those __set_name__ methods are called with the class being defined and the assigned name of that particular attribute

In the case of BaseModel, some names in the original namespace (such as instances of ModelPrivateAttr) are stripped before calling type.__new__ with the new_namespace. As such, objects implementing this __set_name__ protocol are skipped.

Rather than calling type.__new__ with the original namespace, this PR implements the protocol directly by calling __set_name__ on any objects in the namespace that provide the method.

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Context

For context, I've been using sqlmodel recently and would like to to implement this pattern

from sqlmodel import SQLModel

class SomeObject(SQLModel, table=True):
    name: str
    slug: str | None

    @on_before_save
    def _do_stuff(self):
        self.slug = slugify(self.name)

to bind object-specific listeners to various SqlAlchemy ORM events... however, this requires that the decorator know the class in which it is being called (which, among other things, seems to have been a primary motivator of __set_name__)

@tlambert03 tlambert03 changed the title fix: fix PEP487 __set_name__ protocol in BaseModel Fix PEP487 __set_name__ protocol in BaseModel Aug 20, 2022
@tlambert03
Copy link
Contributor Author

please review

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.

otherwise, LGTM.

Please update.

tests/test_create_model.py Outdated Show resolved Hide resolved
tests/test_create_model.py Outdated Show resolved Hide resolved

class A(BaseModel):
@class_deco
def _some_func(self):
Copy link
Member

Choose a reason for hiding this comment

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

can you also call _some_func to check it's a proper bound method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

working on this one still, don't merge yet :)

@samuelcolvin samuelcolvin enabled auto-merge (squash) August 21, 2022 13:47
@samuelcolvin
Copy link
Member

Looks good, thanks so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants