-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
deferred-fields-patch #1064
deferred-fields-patch #1064
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1064 +/- ##
=======================================
Coverage 97.24% 97.25%
=======================================
Files 23 23
Lines 1234 1238 +4
Branches 200 201 +1
=======================================
+ Hits 1200 1204 +4
Misses 16 16
Partials 18 18
... and 10 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Anything I can do to help get this PR accepted? |
simple_history/models.py
Outdated
field_attrs = {field.attname for field in fields} | ||
deferred_attrs = instance.get_deferred_fields() | ||
# Load all deferred fields that are present in fields_included | ||
for attr in field_attrs.intersection(deferred_attrs): |
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.
Maybe it would be better to load all deferred fields in a single query, as Django loads fields one at a time on access (See https://docs.djangoproject.com/en/5.1/ref/models/querysets/#defer)
Updated code may look something like this:
instance.refresh_from_db(fields=field_attrs.intersection(deferred_attrs))
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.
Yes, good idea, I have implemented this. Hopefully this gets reviewed and accepted soon!
Hi @JordanHyatt, |
New pull request #1393 |
Yeah I originally did it on my forks master branch, but I wanted to work on HistoricOneToOneField so I had to open a new PR which it looks like you found, thanks! |
Provided a patch that allows objects to be deleted when they have deferred fields present
Description
Added a pre_delete signal that checks to see if there are any deferred fields on the instance that are present in the included_fileds
Related Issue
This is related to issue #678
Motivation and Context
This allows users of simple_history to delete objects without having to consider the consequences of deferring fields in their query
How Has This Been Tested?
Added a test that confirms objects with deferred fields can in-fact be deleted now
Types of changes
Checklist:
pre-commit run
command to format and lint.AUTHORS.rst
CHANGES.rst