-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add support for Python 3 #373
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
Conversation
Looks like the build fails with DJANGO=1.5 anyway |
I don't think debug_toolbar.utils.tracking._replace_function can ever be used on Python3 because of the removal of .im_self and .im_class The best alternative is to iterate through This means no decorators on local classes, every call will have to be _replace_function(foo, class=foo) |
The issues are:
|
=> python2.6.5
change attributes "func_code" -> "__code__" and "func_closure" -> "__closure__" ( => python2.6 )
Remove u'' prefix, and import __future__ unicode_literals. Now all strings without the u'' - are Unicode. >>> from __future__ import unicode_literals >>> hasattr(str, '__name__') True
Hi! I tested your branch, unfortunately it does have some flaws. |
This commit allows masterbranch to pass all original tests with Django 1.5 (python2). Django 1.5 introduce python's bultins vars "True", "False", "None" to the Context, as part of BaseContext class: """ class BaseContext(object): def __init__(self, dict_=None): self._reset_dicts(dict_) def _reset_dicts(self, value=None): builtins = {'True': True, 'False': False, 'None': None} if value: builtins.update(value) self.dicts = [builtins] """ django/django@93240b7 So, now we always have List's first element that contains these vars. Should we see this in the django-debug-toolbar?
don't allow failures
@jezdez and I think that @robhudson should take a look at this |
@jezdez @robhudson Are there any thoughts on this? This app is very important for django related webapps, so if it would have python 3 support it will be very convenient for new projects. |
@w-diesel We're aware of that and await review from @robhudson. @graingert looks like some changes made to master (the sql panel refactor) broke the merge. Can you double check? |
This doesn't appear to work on Django 1.4.5 for me. I get an import error |
@@ -177,3 +182,33 @@ def get_stack(context=1): | |||
framelist.append((frame,) + getframeinfo(frame, context)) | |||
frame = frame.f_back | |||
return framelist | |||
|
|||
|
|||
class deprecated(object): |
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 usually like to follow the convention that classes start with a capital.
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.
deprecated is a decorator class, and as such is lower case
In debug_toolbar/utils/init.py:
@@ -177,3 +182,33 @@ def get_stack(context=1):
framelist.append((frame,) + getframeinfo(frame, context))
frame = frame.f_back
return framelist
+
+
+class deprecated(object):I usually like to follow the convention that classes start with a capital.
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/373/files#r3960527
.
This is great. Thanks for the awesome work on the port. When I'm on another system I'll take it for a spin on a few projects. |
print ' [%.2fms]' % (ms_from_timedelta(execution_time),) | ||
print(sqlparse.format(raw_sql, reindent=True),) | ||
print(' [%.2fms]' % (ms_from_timedelta(execution_time),)) |
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.
ah, there seems to be some % formatting that needs getting rid of soonish
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.
Nah, old style str subst is okay to have.
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.
IMO
bump |
I've set up a Django 1.5 project to test this out now. This patch no longer merges cleanly, however. Would you mind doing a rebase and fix the merge issues and update the pull request? |
Hi, Thank you for all the hard work. Cheers, |
Maybe I'm missing something obvious but I can't find 1b2caac -- the link works but it's not in the commit log for master. Is it on another branch? |
Hi, |
I reviewed the patch and didn't spot any obvious mistakes. A few comments:
|
I've just renamed this pull request as it's the most up to date effort in porting the debug toolbar to Python 3. The diff is quite large and hard to review. I'm not comfortable with merging it in a single commit. I'm planning to perform a series of commits reducing the gap between the @django-debug-toolbar master, and the @Elvard master. Once the diff gets more manageable, I'll figure out how to close the gap. (Yes, that part of the plan isn't particularly clear.) Does that sound like a reasonable plan? |
I thought this was already merged? |
I don't think so; it was merged to @Elvard's fork but not to the reference repository. Apparently @Elvard's fork has the most recent state of the efforts on this ticket. After adding the appropriate remotes, the diff between his branch and mine can be reviewed with:
(I'm only considering Python files as the other files aren't relevant to porting to Python 3.) |
I've reached a state where my branch appears to run on Python 3, and differences with @Elvard's version seem unrelated to Python 3 support. I've taken a slightly different approach sometimes. @graingert @w-diesel @Elvard, thanks for your efforts on this issues. Your work made it easy for me to rebuild a clean history of changes I feel comfortable committing and supporting. If you want to be credited in AUTHORS, just give me the appropriate line in the form "Name [email]". I'm now going to close this pull request, which cannot be merged cleanly in master, in favor of #412, which does. |
Thank you, perfect! For me, I haven't done anything, just merged @graingert branch to master, so no need for credit. |
@aaugustin Thomas Grainger [django-debug-toolbar@graingert.co.uk] |
No description provided.