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

Migration Django 2.0 #239

Merged
merged 13 commits into from
Nov 19, 2017
Merged

Migration Django 2.0 #239

merged 13 commits into from
Nov 19, 2017

Conversation

stop5
Copy link
Contributor

@stop5 stop5 commented Sep 24, 2017

These commits should add an basic compatibility with django 2.0a1

Updated models.py to support Django 2.0
replaced django.core.urlresolvers with django.urls
@coveralls
Copy link

coveralls commented Sep 24, 2017

Coverage Status

Coverage decreased (-0.7%) to 91.269% when pulling 1a3c0e0 on stop5:master into 1f37350 on idlesign:master.

@idlesign
Copy link
Owner

idlesign commented Sep 25, 2017

Thank you.
We also need to update tox and travis configs, and address backward compatibility issues (for pre 1.10) if any.

this adds compatibility with django prior to 1.10
@coveralls
Copy link

coveralls commented Sep 25, 2017

Coverage Status

Coverage decreased (-0.9%) to 91.129% when pulling f883b64 on stop5:master into 1f37350 on idlesign:master.

replaced django.conf.urls with django.core.urlresolvers.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 92.016% when pulling ad2cc24 on stop5:master into 1f37350 on idlesign:master.

Repository owner deleted a comment from coveralls Sep 26, 2017
Repository owner deleted a comment from coveralls Sep 26, 2017
Repository owner deleted a comment from coveralls Sep 26, 2017
Repository owner deleted a comment from coveralls Sep 26, 2017
Repository owner deleted a comment from coveralls Sep 26, 2017
@idlesign
Copy link
Owner

idlesign commented Nov 3, 2017

Thank you.
Please note that according to Travis builds for new version are failed.

@coveralls
Copy link

coveralls commented Nov 10, 2017

Coverage Status

Coverage decreased (-0.003%) to 91.994% when pulling d55e466 on stop5:master into 99adc29 on idlesign:master.

@@ -63,6 +63,9 @@ def handle(self, *apps, **options):
item.save(using=using)
# Copy permissions to M2M field once `item`
# has been saved
item.access_permissions = item.permissions
if hasattr(item.access_permissions, "set"):
Copy link
Owner

Choose a reason for hiding this comment

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

Here and in other cases please use single quotes ' for string literals to not to contradict overall style and reduce visual noise.

@@ -608,7 +608,13 @@ def get_tree_current_item(self, tree_alias):
return None

# urlquote is an attempt to support non-ascii in url.
current_url = urlquote(self.current_request.path)
path = self.current_request.path
if type(path) == str:
Copy link
Owner

Choose a reason for hiding this comment

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

Here isinstance should be used.

Copy link
Owner

Choose a reason for hiding this comment

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

This branching will cost us something, bearing in mind it's a busy method. To reduce the cost we should try to pick conversion we need (if we really need it) on import time (e.g. partial or something).

current_url = urlquote(path.encode("UTF-8"))
elif type(path) in (bytes, bytearray):
current_url = urlquote(path)
else:
Copy link
Owner

Choose a reason for hiding this comment

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

What type do mean?

@@ -851,7 +857,10 @@ def check_access(self, item, context):
:param Context context:
:rtype: bool
"""
authenticated = self.current_request.user.is_authenticated()
if hasattr(self.current_request.user.is_authenticated, "__call__"):
Copy link
Owner

Choose a reason for hiding this comment

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

It seems that this logic can also be moved onto module import stage.

Repository owner deleted a comment from coveralls Nov 11, 2017
Repository owner deleted a comment from coveralls Nov 11, 2017
Repository owner deleted a comment from coveralls Nov 11, 2017
Repository owner deleted a comment from coveralls Nov 11, 2017
Repository owner deleted a comment from coveralls Nov 11, 2017
Repository owner deleted a comment from coveralls Nov 11, 2017
Repository owner deleted a comment from coveralls Nov 11, 2017
@coveralls
Copy link

coveralls commented Nov 18, 2017

Coverage Status

Coverage increased (+0.07%) to 92.067% when pulling 1500501 on stop5:master into 99adc29 on idlesign:master.

@idlesign idlesign merged commit 1e7345d into idlesign:master Nov 19, 2017
@idlesign
Copy link
Owner

Thank you! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants