Skip to content

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

Closed
wants to merge 34 commits into from
Closed

Conversation

graingert
Copy link
Contributor

No description provided.

@graingert
Copy link
Contributor Author

Looks like the build fails with DJANGO=1.5 anyway

@graingert
Copy link
Contributor Author

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 func.__qualname__.split('.'), calling getattr until just before we hit func. Which if it contains '<locals>' will break and of course will break for instance methods.

This means no decorators on local classes, every call will have to be _replace_function(foo, class=foo)

@graingert
Copy link
Contributor Author

The issues are:

  • need to add test case for utils.tracking.monkey_patch
  • need to fix issues with auth.User on django 1.5 for 3 and 2, probably due to the new user stuff
  • need to test the thing in a real app

=> 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
@w-diesel
Copy link
Contributor

Hi!
Thank you for your work on python3 support!

I tested your branch, unfortunately it does have some flaws.
I submitted corrections to master, now debug_toolbar works the same way in py2 and py3.

w-diesel and others added 3 commits April 23, 2013 16:04
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
@graingert
Copy link
Contributor Author

@jezdez and I think that @robhudson should take a look at this

@w-diesel
Copy link
Contributor

@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.
thanks.

@w-diesel w-diesel mentioned this pull request Apr 25, 2013
@jezdez
Copy link
Contributor

jezdez commented Apr 25, 2013

@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?

@robhudson
Copy link
Contributor

This doesn't appear to work on Django 1.4.5 for me. I get an import error ImportError: cannot import name force_bytes from "debug_toolbar/views.py", line 15.

@@ -177,3 +182,33 @@ def get_stack(context=1):
framelist.append((frame,) + getframeinfo(frame, context))
frame = frame.f_back
return framelist


class deprecated(object):
Copy link
Contributor

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.

Copy link
Contributor Author

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
.

@robhudson
Copy link
Contributor

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
print(sqlparse.format(raw_sql, reindent=True),)
print(' [%.2fms]' % (ms_from_timedelta(execution_time),))
Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO

@graingert
Copy link
Contributor Author

bump

@robhudson
Copy link
Contributor

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?

@tricoder42
Copy link

Hi,
I'm also using Python 3 with Django 1.5, so I merged this branch and resolved commits. See 1b2caac.

Thank you for all the hard work.

Cheers,
Tom

@ryankask
Copy link
Contributor

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?

@tricoder42
Copy link

Hi,
sorry, I've created branch py3k to play with. I've just pushed it into master.
See 1b2caac or https://github.com/elvard/django-debug-toolbar/commits/master

@aaugustin
Copy link
Contributor

I reviewed the patch and didn't spot any obvious mistakes. A few comments:

  • I really encourage adding from __future__ import unicode_literals to each and every Python file. Having to check the top of the file to determine if 'foo' is a bytestring or a unicode string is very annoying.
  • I didn't check whether functions marked with not_on_py3 are actually impossible to implement on Python 3.

@aaugustin
Copy link
Contributor

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?

@graingert
Copy link
Contributor Author

I thought this was already merged?

@aaugustin
Copy link
Contributor

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:

find debug_toolbar -name \*.py -print0 | xargs -0 git diff aaugustin/python3 elvard/master

(I'm only considering Python files as the other files aren't relevant to porting to Python 3.)

@aaugustin
Copy link
Contributor

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.

@aaugustin aaugustin closed this Oct 17, 2013
@tricoder42
Copy link

Thank you, perfect!

For me, I haven't done anything, just merged @graingert branch to master, so no need for credit.

@graingert
Copy link
Contributor Author

@aaugustin Thomas Grainger [django-debug-toolbar@graingert.co.uk]

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.

7 participants