-
-
Notifications
You must be signed in to change notification settings - Fork 476
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
Ensure custom arguments for fields are included in historical models' fields #462
Conversation
Codecov Report
@@ Coverage Diff @@
## master #462 +/- ##
==========================================
+ Coverage 97.09% 97.19% +0.09%
==========================================
Files 15 16 +1
Lines 689 713 +24
Branches 96 94 -2
==========================================
+ Hits 669 693 +24
Misses 10 10
Partials 10 10
Continue to review full report at Codecov.
|
simple_history/models.py
Outdated
@@ -178,29 +178,18 @@ def copy_fields(self, model): | |||
field.__class__ = models.IntegerField | |||
if isinstance(field, models.ForeignKey): | |||
old_field = field | |||
field_arguments = {'db_constraint': False} | |||
old_field.swappable = False | |||
name, path, args, field_arguments = old_field.deconstruct() |
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.
Make name
and path
underscores since they aren't used?
@@ -178,29 +178,18 @@ def copy_fields(self, model): | |||
field.__class__ = models.IntegerField | |||
if isinstance(field, models.ForeignKey): | |||
old_field = field | |||
field_arguments = {'db_constraint': False} | |||
old_field.swappable = False |
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.
Let's change this back to its original setting after calling deconstruct
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.
@tbeadle let's change the swappable setting back after doing this deconstruct? And I'd love to figure out why those fields need to be sorted in tests – haven't been able to find why the order changes but want to know before merging. I'll look into it some more
… fields. Previously, if a custom field class was used for a foreign key and that class accepted custom kwargs, those kwargs would not be included in the creation of the historical model's field and would break the migration. To address this, we'll use the deconstruct() method of the field to get the arguments that should be passed to it and override the ones we need to for the historical model's field.
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.
lgtm thanks @tbeadle
Description
Previously, if a custom field class was used for a foreign key and that
class accepted custom kwargs, those kwargs would not be included in the
creation of the historical model's field and would break the migration.
To address this, we'll use the deconstruct() method of the field to get
the arguments that should be passed to it and override the ones we need
to for the historical model's field.
Related Issue
Fixes #431
Motivation and Context
See description
How Has This Been Tested?
Added unit tests
Types of changes
Checklist: