-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Use related_objects api for Django 1.9+ #3656
Conversation
@@ -146,7 +152,11 @@ def _get_reverse_relationships(opts): | |||
) | |||
|
|||
# Deal with reverse many-to-many relationships. | |||
for relation in opts.get_all_related_many_to_many_objects(): | |||
if django.VERSION < (1, 9): |
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 sort of version-branching code should go into combat.py
We'll want eg a get_all_related_objects(opt)
function, and a get_all_related_many_to_many_objects(opt)
function.
Looking good tho!
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.
@tomchristie Yeah, I started so, but then decided to move it to the only place where it's used and there are already some compat things in the method. Do you want to move all compat-related things to compat.py
or just the latest addition?
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.
It's probably better to use django version check instead of getattr
. What do you think, should I change it?
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.
Do you want to move all compat-related things to compat.py or just the latest addition?
Just stuff that has a version switch.
It's probably better to use django version check instead of getattr.
Agree. The version check is fine.
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.
@tomchristie With version check I meant the lines which are already there, e.g.
related = getattr(relation, 'related_model', relation.model)
Do you prefer to move them as well e.g. related_model(opts)
?
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.
If there's a newer 1.9 API that we should be using instead then yes. Otherwise no.
9b905b4
to
2acc6a7
Compare
@tomchristie done. |
Perfect! |
Fix #3252 -- Use related_objects api for Django 1.9+
Closes #3252.