Skip to content

Conversation

hugorodgerbrown
Copy link
Contributor

Provides dual-support for Django 1.11 and 2.0, drops support for python2.

  • Convert is_authenticated(), is_anonymous() to properties
  • Add on_delete to all foreign keys, and update migrations
  • Update urls.py to handle 1.11 and 2.0 re_path format
  • Update RequestTokenMiddleware to drop use of MiddlewareMixin in favour of new style
  • Remove use of six

@hugorodgerbrown hugorodgerbrown requested a review from djm February 28, 2018 12:05
def _auth_is_anonymous(self, request):
"""Authenticate anonymous requests."""
assert request.user.is_anonymous(), 'User is authenticated.'
assert request.user.is_anonymous, 'User is authenticated.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is bad. assert can be stripped by the compiler so this code could effectively be a pass on those that do so. We don't run with compiler optimisations stripping the asserts, but as this is OS I feel we have a little duty to fix this up.

Can be replaced with a conditional instead, which never get stripped.

More: https://dbader.org/blog/python-assert-tutorial

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True - but not part of this PR.

def _auth_is_authenticated(self, request):
"""Authenticate requests with existing users."""
assert request.user.is_authenticated(), 'User is anonymous.'
assert request.user.is_authenticated, 'User is anonymous.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto ^

@hugorodgerbrown
Copy link
Contributor Author

@djm asserts removed

@hugorodgerbrown hugorodgerbrown merged commit e987002 into master Feb 28, 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.

2 participants