-
Notifications
You must be signed in to change notification settings - Fork 137
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 Django deprecation warning #165
Fix Django deprecation warning #165
Conversation
Thanks for the PR! I'll take a look. |
rollbar/contrib/django/middleware.py
Outdated
from django.conf import settings | ||
from django.http import Http404 | ||
|
||
if django.VERSION >= (1, 10): |
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.
Hmm, I'm not sure if this is the right way to do this particular comparison. django.VERSION
usually has text in it. For example, (1, 11, 1, u'final', 0)
.
I know this is probably not necessary and really corner case-y, but I think it might be better to do an import check similar to how we handle MiddlewareMixin
below.
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.
I'm happy to change it but checking the version is a pretty common pattern:
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 link. That's definitely something to consider since a method could change to a property and vice-versa. I'm going to file an internal ticket to discuss with the team.
Updated to use try/except. |
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.
Looks good! Thanks for contributing!
I filed an internal ticket for us to discuss selecting version-specific behavior using django.VERSION
in the future.
No description provided.