-
-
Notifications
You must be signed in to change notification settings - Fork 483
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 excluded_field_kwargs to support custom OneToOneFields. #871
Add excluded_field_kwargs to support custom OneToOneFields. #871
Conversation
I didn't check the tests case because I did not run the entire testing matrix locally. |
@@ -198,13 +199,21 @@ def test_registering_with_tracked_abstract_base(self): | |||
|
|||
|
|||
class TestCustomAttrForeignKey(TestCase): | |||
""" https://github.com/jazzband/django-simple-history/issues/431 """ | |||
"""https://github.com/jazzband/django-simple-history/issues/431""" |
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.
This was a result of make format
.
@@ -219,7 +228,7 @@ def test_migrate_command(self): | |||
|
|||
|
|||
class TestModelWithHistoryInDifferentApp(TestCase): | |||
""" https://github.com/jazzband/django-simple-history/issues/485 """ | |||
"""https://github.com/jazzband/django-simple-history/issues/485""" |
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.
This was a result of make format
.
Coercing OneToOneFields to be ForeignKeys on historical models causes issues when the OneToOneField is a custom subclass that includes additional arguments that do not exist on the ForeignKey.__init__ method. These additional arguments can be specified via excluded_field_kwargs to allow these custom OneToOneFields be coerced into ForeignKeys on the historical model. Fixes jazzband#870
b2e2508
to
7759125
Compare
And I named the branch incorrectly... It should have been 870, not 807. |
Codecov Report
@@ Coverage Diff @@
## master #871 +/- ##
==========================================
- Coverage 97.85% 97.65% -0.20%
==========================================
Files 20 19 -1
Lines 1025 1023 -2
Branches 159 154 -5
==========================================
- Hits 1003 999 -4
Misses 10 10
- Partials 12 14 +2
Continue to review full report at Codecov.
|
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.
Documentation and unit tests - looks good. Looks like there's one additional branch that did not get covered in unit tests. Need to rebase and fix up the collision in CHANGES. If this gets in before my other two I will rebase them to fix the resulting migration collisions.
@tim-schilling there's probably just one more branch that needs coverage here, and a rebase to fix up the changes file. |
Support custom OneToOneFields that specify additional arguments that don't exist in
ForeignKey.__init__
.Description
These additional arguments can be specified via excluded_field_kwargs to allow these custom OneToOneFields be coerced into ForeignKeys on the historical model.
This adds the
excluded_field_kwargs
parameter toHistoricalRecords
. It is a dictionary mapping fields to list of parameters that should be excluded when crafting the ForeignKey instance for the historical model.Related Issue
Fixes #870
To reproduce, the following model can be added and then attempt to run
makemigrations
andmigrate
.Motivation and Context
Coercing OneToOneFields to be ForeignKeys on historical models causes issues when the OneToOneField is a custom subclass that includes additional arguments that do not exist on the
ForeignKey.__init__
method.How Has This Been Tested?
The problem was able to be reproduced by adding
CustomAttrNameOneToOneField
andModelWithCustomAttrOneToOneField
to the test app. There's an additional test that verifies the constraint of this change in that the additional arguments are not available on the ForeignKey field. This is done viaTestCustomAttrOneToOneField
.Screenshots (if appropriate):
N/A
Types of changes
Checklist:
make format
command to format my codeAUTHORS.rst
CHANGES.rst