-
-
Notifications
You must be signed in to change notification settings - Fork 300
Fixed custom __init__ check in transmogrify #614
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
Conversation
f2f31a1 to
e34db88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug in the polymorphic object retrieval mechanism where custom __init__ methods were not being called when transmogrifying objects between related classes. The issue occurred because the code was checking for __init__ in the instance's __dict__ (which would never contain class methods) instead of the class's __dict__. The fix changes the check from obj.__dict__ to obj.__class__.__dict__, ensuring that custom initialization logic is properly executed when converting polymorphic instances.
Key Changes:
- Fixed the
__init__existence check in thetransmogrify()function to correctly identify custom initialization methods - Added test models (
BlueHeadDuck,HomeDuck,PurpleHeadDuck) to demonstrate the bug scenario with proxy models and multiple inheritance - Added a regression test to ensure custom
__init__methods are called during polymorphic object retrieval
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/polymorphic/query.py |
Fixed the bug by changing the __init__ check from obj.__dict__ to obj.__class__.__dict__ in the transmogrify() function |
src/polymorphic/tests/models.py |
Added test models (BlueHeadDuck, HomeDuck, PurpleHeadDuck) to support testing the fix with proxy models and custom __init__ methods |
src/polymorphic/tests/test_orm.py |
Added test_transmogrify_with_init test case and updated imports to include new test models |
src/polymorphic/tests/migrations/0001_initial.py |
Regenerated migration file to include the new test models (Duck, BlueHeadDuck, PurpleHeadDuck) |
Comments suppressed due to low confidence (1)
src/polymorphic/tests/test_orm.py:1416
- Normal methods should have 'self', rather than 'db', as their first parameter.
def test_transmogrify_with_init(db):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #614 +/- ##
==========================================
+ Coverage 74.96% 75.40% +0.44%
==========================================
Files 21 21
Lines 1342 1342
Branches 211 211
==========================================
+ Hits 1006 1012 +6
+ Misses 261 257 -4
+ Partials 75 73 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9412de5 to
59cfde0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The custom __init__ transmogrify code was dead because it erroneously looked at the object dict to locate an __init__ implementation instead of the class __dict__.
I think there is still an edge case here where values set differently by the base class and subclass's __init__ may be overridden after the subclass instantiation - but its hard to solve for that and this code is certainly more correct.
Thank you!
… object. The __init__ wasn't called previously. - Added unit-tests for jazzband#615 - update changelog Co-authored-by: Bartosz Dabrowski <bartosz.dabrowski@adverity.com> Co-authored-by: Brian Kohan <bckohan@gmail.com>
59cfde0 to
8b11bf0
Compare
Fixes #615