Skip to content
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 various deprecation warnings #2222

Merged
merged 11 commits into from Feb 5, 2017
Merged

Fix various deprecation warnings #2222

merged 11 commits into from Feb 5, 2017

Conversation

mvantellingen
Copy link
Contributor

Would be good to get this in before 1.4.

It fixes the following (1500) deprecation warnings:

  • RemovedInDjango20Warning: Using user.is_authenticated() and user.is_anonymous() as a method is deprecated. Remove the parentheses to use it as an attribute.
  • ResourceWarning: unclosed file ..
  • Direct assignment to the forward side of a many-to-many set is deprecated due to the implicit save() that happens. Use offers.set() instead.

It also ignores warnings in django-webtest and sorl, see setup.cfg

The last few should be fixed by #2157 (django-tables2)

@mvantellingen mvantellingen added this to the 1.4 milestone Feb 5, 2017
if django.VERSION >= (1, 10):
return user.is_authenticated
else:
return user_is_authenticated(user)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be return user.is_authenticated()? This will trigger an infinite loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, search-and-replace gone wrong. Thanks :-)

@codecov-io
Copy link

codecov-io commented Feb 5, 2017

Codecov Report

Merging #2222 into master will increase coverage by 0.04%.

@@            Coverage Diff             @@
##           master    #2222      +/-   ##
==========================================
+ Coverage   81.82%   81.87%   +0.04%     
==========================================
  Files         326      326              
  Lines       15569    15604      +35     
==========================================
+ Hits        12740    12775      +35     
  Misses       2829     2829
Impacted Files Coverage Δ
.../apps/customer/notifications/context_processors.py 100% <100%> (ø)
src/oscar/apps/voucher/abstract_models.py 89.15% <100%> (ø)
src/oscar/apps/dashboard/app.py 100% <100%> (ø)
src/oscar/core/compat.py 90.52% <100%> (+1.36%)
src/oscar/templatetags/basket_tags.py 95% <100%> (+0.26%)
src/oscar/apps/catalogue/views.py 92% <100%> (+0.06%)
src/oscar/apps/basket/middleware.py 82.47% <100%> (ø)
src/oscar/apps/catalogue/app.py 100% <100%> (ø)
src/oscar/apps/checkout/mixins.py 80.14% <100%> (+0.14%)
src/oscar/templatetags/category_tags.py 100% <100%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17e5a11...534d1a6. Read the comment docs.

if id:
yield id
with open(self.filepath, 'r') as fh:
for line in fh.readlines():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readlines() loads the entire contents of the file into memory. I think it's better to just loop over the file object itself: for line in fh which is memory-efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

'django.contrib.sessions.middleware.SessionMiddleware',
'django.middleware.csrf.CsrfViewMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'django.contrib.auth.middleware.SessionAuthenticationMiddleware',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SessionAuthenticationMiddleware no longer does anything and is removed in Django 2.0 - can remove it from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, thanks

@mvantellingen mvantellingen merged commit d04a487 into master Feb 5, 2017
@mvantellingen mvantellingen deleted the deprecation-warnings branch February 5, 2017 11:04
MrVoltz pushed a commit to EndevelCZ/django-oscar that referenced this pull request Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants