Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add support for Python 3 #373

Closed
wants to merge 34 commits into from

7 participants

@graingert

No description provided.

@graingert

Looks like the build fails with DJANGO=1.5 anyway

@graingert

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

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
w-diesel added some commits
@w-diesel w-diesel fix print function
=> python2.6.5
47ea676
@w-diesel w-diesel Update profiling.py, attr. "func_closure" ( py2.6)
change attributes "func_code" -> "__code__" and "func_closure" -> "__closure__"   ( => python2.6 )
76a8159
@w-diesel w-diesel remove u'' prefix, import __future__ unicode_lit..
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
f5d4cde
@w-diesel

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.

@graingert

@w-diesel I've added your commits into this PR

@graingert

clearly the test cases are not good enough as I only changed to pass the test cases, @w-diesel can you add test cases for the changes you've made?

@w-diesel

I'll see what could be done with tests.

tests/tests.py
((120 lines not shown))
-
- self.assertTrue('sender' in foo, foo)
- # best we can do
- self.assertEquals(foo['sender'].__name__, 'class_func')
- self.assertTrue('start' in foo, foo)
- self.assertTrue(foo['start'] > 0)
- self.assertTrue('stop' in foo, foo)
- self.assertTrue(foo['stop'] > foo['start'])
- self.assertTrue('args' in foo, foo)
- self.assertTrue(len(foo['args']), 2)
- self.assertEquals(foo['args'][1], 'hello')
- self.assertTrue('kwargs' in foo, foo)
- self.assertTrue(len(foo['kwargs']), 1)
- self.assertTrue('foo' in foo['kwargs'])
- self.assertEquals(foo['kwargs']['foo'], 'bar')
+if not six.PY3:
@jezdez Owner
jezdez added a note

Could you use the skipIf decorator here?

@unittest.skipIf(six.PY3, 'only works on Python 2.x')
class TrackingTestCase(BaseTestCase):
    # ...

I don't think that's supported in python 2.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jezdez
Owner

@graingert Can you pull the latest changes from #374?

w-diesel and others added some commits
@w-diesel w-diesel cache.py, fix python3.2 support 7a21c54
@w-diesel w-diesel Update .travis.yml
don't allow failures
ee44437
@w-diesel w-diesel Masterbranch can pass all original tests on 1.5ver
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?
fa40aa1
@graingert graingert use unittest.skipIf to skip tests invalid on PY3 fd2bb66
@graingert

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

@w-diesel

@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 referenced this pull request
Closed

Python 3 support #339

@jezdez
Owner

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

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.

@robhudson robhudson commented on the diff
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):
@robhudson Owner

I usually like to follow the convention that classes start with a capital.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhudson robhudson commented on the diff
debug_toolbar/utils/sqlparse/__init__.py
@@ -1,55 +0,0 @@
@robhudson Owner

This is probably for the best. I think I included it to avoid an extra dependency. But the world of Python packaging has gotten better since then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhudson robhudson commented on the diff
debug_toolbar/views.py
@@ -12,6 +12,8 @@
from debug_toolbar.utils.compat.db import connections
+from django.utils.encoding import force_bytes
@robhudson Owner

This fails on Django 1.4.5. I fixed it locally by wrapping it something like this:

try:
    from django.utils.encoding import force_bytes as force_str
except ImportError:
    from django.utils.encoding import force_unicode as force_str

Are you sure, the travis config uses 1.4.5

@graingert "force_bytes" only present in Django 1.5
I did not notice that you have changed my commit, since I only checked my branch in real app(also with mezzanine cms). IMO Maybe we don't need "force_bytes"?
w-diesel@3c1bf7a

@jezdez Owner
jezdez added a note

No, force_bytes was added in 1.5.x since it's required for Python 3.

@aaugustin Owner

I backported a few functions in 1.4.2, but not force_bytes, because Django 1.4 doesn't have force_str.

You may use smart_bytes which is available in Django >= 1.4.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
debug_toolbar/management/commands/debugsqlshell.py
@@ -14,8 +16,8 @@ def execute(self, sql, params=()):
finally:
raw_sql = self.db.ops.last_executed_query(self.cursor, sql, params)
execution_time = datetime.now() - starttime
- print sqlparse.format(raw_sql, reindent=True),
- print ' [%.2fms]' % (ms_from_timedelta(execution_time),)
- print
+ print( sqlparse.format(raw_sql, reindent=True),)
+ print( ' [%.2fms]' % (ms_from_timedelta(execution_time),))
@robhudson Owner

Could remove the extra spacing after the print( in the above 2 lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhudson robhudson commented on the diff
debug_toolbar/panels/sql.py
@@ -195,7 +199,7 @@ def process_response(self, request, response):
})
-class BoldKeywordFilter(sqlparse.filters.Filter):
+class BoldKeywordFilter():
@robhudson Owner

I realize the Filter class has process as its only method, but it still seems reasonable to subclass it IMO. Did this cause a Python 3 problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhudson
Owner

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.

@graingert

We're going to have to pin a version range here. Any suggestions?

@graingert graingert commented on the diff
debug_toolbar/management/commands/debugsqlshell.py
@@ -14,8 +16,8 @@ def execute(self, sql, params=()):
finally:
raw_sql = self.db.ops.last_executed_query(self.cursor, sql, params)
execution_time = datetime.now() - starttime
- print sqlparse.format(raw_sql, reindent=True),
- print ' [%.2fms]' % (ms_from_timedelta(execution_time),)
- print
+ print(sqlparse.format(raw_sql, reindent=True),)
+ print(' [%.2fms]' % (ms_from_timedelta(execution_time),))

ah, there seems to be some % formatting that needs getting rid of soonish

@jezdez Owner
jezdez added a note

Nah, old style str subst is okay to have.

@jezdez Owner
jezdez added a note

IMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@graingert

bump

@robhudson
Owner

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

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

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

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
Owner

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
Owner

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

I thought this was already merged?

@aaugustin
Owner

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
Owner

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

Thank you, perfect!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 8, 2013
  1. @graingert
  2. @graingert

    add <1.6 to tests_require

    graingert authored
  3. @graingert

    use sqlparse from pypi

    graingert authored
  4. @graingert
  5. @graingert
  6. @graingert
  7. @graingert
  8. @graingert
  9. @graingert
  10. @graingert
  11. @graingert
  12. @graingert
  13. @graingert
  14. @graingert
  15. @graingert
  16. @graingert
Commits on Apr 9, 2013
  1. @graingert
  2. @graingert
  3. @graingert

    use map from six

    graingert authored
  4. @graingert
  5. @graingert
  6. @graingert
Commits on Apr 14, 2013
  1. @w-diesel

    fix print function

    w-diesel authored
    => python2.6.5
  2. @w-diesel

    Update profiling.py, attr. "func_closure" ( py2.6)

    w-diesel authored
    change attributes "func_code" -> "__code__" and "func_closure" -> "__closure__"   ( => python2.6 )
  3. @w-diesel

    remove u'' prefix, import __future__ unicode_lit..

    w-diesel authored
    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
  4. @graingert
  5. @w-diesel @graingert

    list expected

    w-diesel authored graingert committed
  6. @w-diesel @graingert

    Update middleware.py

    w-diesel authored graingert committed
    from __future__ import unicode_literals
Commits on Apr 23, 2013
  1. @w-diesel @graingert

    cache.py, fix python3.2 support

    w-diesel authored graingert committed
  2. @w-diesel @graingert

    Update .travis.yml

    w-diesel authored graingert committed
    don't allow failures
  3. @w-diesel @graingert

    Masterbranch can pass all original tests on 1.5ver

    w-diesel authored graingert committed
    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?
  4. @graingert
Commits on Apr 25, 2013
  1. @graingert
  2. @graingert
Something went wrong with that request. Please try again.