Add support for Python 3 #373

Closed
wants to merge 34 commits into
from

Conversation

Projects
None yet
7 participants
Contributor

graingert commented Apr 8, 2013

No description provided.

Contributor

graingert commented Apr 8, 2013

Looks like the build fails with DJANGO=1.5 anyway

Contributor

graingert commented Apr 9, 2013

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)

Contributor

graingert commented Apr 9, 2013

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 Apr 14, 2013

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

w-diesel commented Apr 14, 2013

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.

Contributor

graingert commented Apr 14, 2013

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

Contributor

graingert commented Apr 14, 2013

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?

Contributor

w-diesel commented Apr 15, 2013

I'll see what could be done with tests.

@jezdez jezdez and 1 other commented on an outdated diff Apr 23, 2013

tests/tests.py
-
- 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

jezdez Apr 23, 2013

Owner

Could you use the skipIf decorator here?

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

graingert Apr 23, 2013

Contributor

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

Owner

jezdez commented Apr 23, 2013

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

w-diesel and others added some commits Apr 15, 2013

@w-diesel @graingert w-diesel cache.py, fix python3.2 support 7a21c54
@w-diesel @graingert w-diesel Update .travis.yml
don't allow failures
ee44437
@w-diesel @graingert 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
Contributor

graingert commented Apr 23, 2013

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

Contributor

w-diesel commented Apr 25, 2013

@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 referenced this pull request Apr 25, 2013

Closed

Python 3 support #339

Owner

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?

Contributor

robhudson commented Apr 25, 2013

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 Apr 25, 2013

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

robhudson Apr 25, 2013

Contributor

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

@graingert

graingert Apr 25, 2013

Contributor

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/django-debug-toolbar/django-debug-toolbar/pull/373/files#r3960527
.

@robhudson robhudson commented on the diff Apr 25, 2013

debug_toolbar/utils/sqlparse/__init__.py
@@ -1,55 +0,0 @@
@robhudson

robhudson Apr 25, 2013

Contributor

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.

@robhudson robhudson commented on the diff Apr 25, 2013

debug_toolbar/views.py
@@ -12,6 +12,8 @@
from debug_toolbar.utils.compat.db import connections
+from django.utils.encoding import force_bytes
@robhudson

robhudson Apr 25, 2013

Contributor

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

graingert Apr 25, 2013

Contributor

Are you sure, the travis config uses 1.4.5

@w-diesel

w-diesel Apr 26, 2013

Contributor

@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/django-debug-toolbar@3c1bf7a

@graingert

graingert Apr 26, 2013

Contributor

I was under the impression that force_bytes had been backported to Django
1.4.5: it seems the tests pass.
On Apr 26, 2013 7:56 AM, "Wladimir Diedel" notifications@github.com wrote:

In debug_toolbar/views.py:

@@ -12,6 +12,8 @@

from debug_toolbar.utils.compat.db import connections

+from django.utils.encoding import force_bytes

@graingert https://github.com/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@3c1bf7ahttps://github.com/w-diesel/django-debug-toolbar/commit/3c1bf7a421f1337bff6f78dfb739642ed47088c5


Reply to this email directly or view it on GitHubhttps://github.com/django-debug-toolbar/django-debug-toolbar/pull/373/files#r3972138
.

@jezdez

jezdez Apr 26, 2013

Owner

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

@aaugustin

aaugustin Sep 16, 2013

Contributor

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.

@robhudson robhudson commented on an outdated diff Apr 25, 2013

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

robhudson Apr 25, 2013

Contributor

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

@robhudson robhudson commented on the diff Apr 25, 2013

debug_toolbar/panels/sql.py
@@ -195,7 +199,7 @@ def process_response(self, request, response):
})
-class BoldKeywordFilter(sqlparse.filters.Filter):
+class BoldKeywordFilter():
@robhudson

robhudson Apr 25, 2013

Contributor

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?

@graingert

graingert Apr 25, 2013

Contributor

Filter no longer exists in sqlparse
On Apr 25, 2013 7:29 PM, "Rob Hudson" notifications@github.com wrote:

In debug_toolbar/panels/sql.py:

@@ -195,7 +199,7 @@ def process_response(self, request, response):
})

-class BoldKeywordFilter(sqlparse.filters.Filter):
+class BoldKeywordFilter():

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?


Reply to this email directly or view it on GitHubhttps://github.com/django-debug-toolbar/django-debug-toolbar/pull/373/files#r3963165
.

Contributor

robhudson commented Apr 25, 2013

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.

Owner

graingert commented on setup.py in 005ca0b Apr 25, 2013

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

@graingert graingert commented on the diff Apr 25, 2013

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),))
@graingert

graingert Apr 25, 2013

Contributor

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

@jezdez

jezdez May 8, 2013

Owner

Nah, old style str subst is okay to have.

Contributor

graingert commented May 16, 2013

bump

Contributor

robhudson commented Jun 9, 2013

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,
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

Contributor

ryankask commented Jun 23, 2013

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,
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

Contributor

aaugustin commented Sep 16, 2013

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.
Contributor

aaugustin commented Oct 16, 2013

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?

Contributor

graingert commented Oct 16, 2013

I thought this was already merged?

Contributor

aaugustin commented Oct 16, 2013

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.)

Contributor

aaugustin commented Oct 17, 2013

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 closed this Oct 17, 2013

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