-
Notifications
You must be signed in to change notification settings - Fork 111
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: add param from_queryset
to refresh_from_db
#235
base: develop
Are you sure you want to change the base?
Conversation
django updated their signature for `refresh_from_db` to include this param, so the library fails on django 5.1 link to django release note: https://docs.djangoproject.com/en/5.1/releases/5.1/ link to django source where it changed: django/django@f92641a
this is kinda hacky, but I cannot think of a better way to do this. I could use inspect to check if `from_queryset` param exists for `refresh_from_db` method, but I believe that is an overkill here.
fa646be
to
e331508
Compare
pinging @romgar for review :) |
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.
Thanks for the PR! Sorry I didn't get to review sooner
Could you also add a regression test for calling refresh_from_db with the from_queryset arg?
this is done to leave transparently pass through all the arguments to django. Makes the function more resiliant against future changes to django's API.
@romgar I fixed the comments, and added a regression test. idk how to write a proper test for this case though, thoughts? |
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.
Thanks, that test is enough, we just want to check the arg can be passed without an error. It does need a @pytest.mark.skipif DJANGO < 5.1 check on it
tests/test_core.py
Outdated
@@ -214,6 +214,23 @@ def test_refresh_from_db_no_fields(): | |||
assert tm.get_dirty_fields() == {"boolean": True, "characters": "old value"} | |||
|
|||
|
|||
@pytest.mark.django_db | |||
def test_refresh_from_db_with_from_queryset(): | |||
"""Tests passthrough of `save_queryset` field in refresh_from_db |
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.
save_queryset -> from_queryset typo
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.
ugh thanks! fixed.
django updated their signature for
refresh_from_db
to include this param, so the library fails on django 5.1link to django release note:
https://docs.djangoproject.com/en/5.1/releases/5.1/
link to django source where it changed:
django/django@f92641a