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

Django 1.10 support #168

Merged
merged 7 commits into from
Jan 8, 2017
Merged

Django 1.10 support #168

merged 7 commits into from
Jan 8, 2017

Conversation

ddc67cd
Copy link
Contributor

@ddc67cd ddc67cd commented Jan 6, 2017

It seems to work fine.

Copy link
Collaborator

@remik remik left a comment

Choose a reason for hiding this comment

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

General good cleanup.

@@ -186,6 +186,9 @@ def language_mapping(lang):
COVERAGE_BRANCH_COVERAGE = False
PAGE_ENABLE_TESTS = True

PASSWORD_HASHERS = [
'django.contrib.auth.hashers.SHA1PasswordHasher'
Copy link
Collaborator

Choose a reason for hiding this comment

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

django.contrib.auth.hashers.MD5PasswordHasher would be faster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, but test fixtures contains SHA1 hashes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could change it. I like changes to speedup development.
This is of course optional.

I was not involve in project for long time so will let @batiste to review it.

Copy link
Owner

Choose a reason for hiding this comment

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

Apart from the change of PAGE_USE_SITE_ID it looks all good to me.

@remik remik requested a review from batiste January 6, 2017 00:58
Copy link
Owner

@batiste batiste left a comment

Choose a reason for hiding this comment

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

PAGE_USE_SITE_ID change is a breaking change and needs to be justified, and pages/settings.py needs to be changed accordingly.

@@ -51,7 +51,7 @@ def on_site(self, site_id=None):

:param site_id: specify the id of the site object to filter with.
"""
if settings.PAGE_USE_SITE_ID:
if global_settings.PAGE_USE_SITE_ID:
Copy link
Owner

Choose a reason for hiding this comment

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

This will force the user to setup the PAGE_USE_SITE_ID setting isn't it? Is this really what we want?

@@ -186,6 +186,9 @@ def language_mapping(lang):
COVERAGE_BRANCH_COVERAGE = False
PAGE_ENABLE_TESTS = True

PASSWORD_HASHERS = [
'django.contrib.auth.hashers.SHA1PasswordHasher'
Copy link
Owner

Choose a reason for hiding this comment

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

Apart from the change of PAGE_USE_SITE_ID it looks all good to me.

@ddc67cd
Copy link
Contributor Author

ddc67cd commented Jan 7, 2017

I've fixed the issue with default settings.

@batiste batiste merged commit 63b44bb into batiste:master Jan 8, 2017
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.

3 participants