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

python3 compatibility fixes #41

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

gbezyuk
Copy link

@gbezyuk gbezyuk commented Oct 11, 2015

No description provided.

@gbezyuk
Copy link
Author

gbezyuk commented Oct 11, 2015

There are some weird changes like extra spaces or "changing text with exactly the same one", I tried to get rid of them but did not succeed. Anyway, it shouldn't matter.

def get_changelist(self, request, **kwargs):
"""
Returns the ChangeList class for use on the changelist page.
"""
return MPTTChangeList

# In Django 1.1, the changelist class is hard coded in changelist_view, so
# we've got to override this too, just to get it to use our custom ChangeList
if django.VERSION < (1, 2):
Copy link
Contributor

Choose a reason for hiding this comment

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

Who is still using Django 1.1? Wow! -- @gbezyuk I think we should remove the whole code portion all together.

@bittner
Copy link
Contributor

bittner commented Oct 12, 2015

Just a comment: How can we make sure these changes actually make django-media-tree work with Python 3? And then again, which versions of Python 3?

@samluescher I had added tox configuration and badges in PR #40, but in fact there are no tests to run. So, we can't actually test the code, be it existing code or changes, against any target.

Can we come up with a very global function test covering a lot of media tree's feature set, with limited effort? (i.e. no unit tests, they can come later)

@gbezyuk
Copy link
Author

gbezyuk commented Oct 12, 2015

@bittner there are no tests to check the project with, that's true. Actually, I came with these changes while trying to run django-media-tree against python3 and django1.8.4. (Didn't succeed with this version of Django yet, BTW — thus there are no django-related changes in this PR.)

There are only three types of changes in this PR:

  • exception handling syntax (ExceptionType 'as' exception, not ExceptionType, exception)
  • unicode-related imports (smart_text instead of smart_unicode, keeping python2 names in the rest of a code)
  • minor syntax fixes like print arg to pring(arg) and unichr() to from builtins import chr <...> chr()

All of these at least shouldn't break anything, and clearly moves the codebase closer to python3 readiness — due to absence of tests, unfortunately I can't help with much more.

@bittner
Copy link
Contributor

bittner commented Oct 12, 2015

@gbezyuk I don't doubt it. Really, I love seeing projects move closer to Python 3. Though, how can we make sure media tree remains compatible with all supported Python 2 (and Django) versions? Without any tests? We can't.

Have you tried running the demo project included in the repository? (I must admit, I haven't.) -- I'd suggest to build the demo project for each and every Python-Django combination and run basic function tests [1] against them. That shouldn't be too much of a pain. The tox configuration is already in place. And Travis-CI is waiting for another open source project to burn their CPU power! 😏

[1] Running python manage.py migrate and python manage.py check would already be "tests"!

@gbezyuk
Copy link
Author

gbezyuk commented Oct 12, 2015

Well, actually with python3 the demo project fails on pip install -r requirements step with errors like print "Setuptools version",version,"or greater has been installed." SyntaxError: Missing parentheses in call to 'print' while installing wsgiref and easy-thumbnails. Can't do much with this without hacking deep into the project, too.

With python2.7, however, it still works as expected, so I doubt if I broke anything.

@bittner
Copy link
Contributor

bittner commented Oct 12, 2015

Don't worry, I'm not blaming you on braking anything. We should make the demo project deploy (as a test) on Travis-CI then we're all set. I'll try. -- Thanks for the discussion!

@gbezyuk
Copy link
Author

gbezyuk commented Oct 12, 2015

Thank you too) hope I triggered some good changes ;)

@SebCorbin
Copy link

I would also add this for python 3.5 compatibility

try:
    from builtins import chr as unichr
except ImportError:
    pass

@SebCorbin SebCorbin mentioned this pull request Nov 4, 2015
@bittner
Copy link
Contributor

bittner commented Jan 6, 2016

@gbezyuk The print complaints are easily fixed in the demo project. We just need to use print() as a function instead of a keyword. Works both in 2.7 and 3.x. Ooops, not if it comes from incompatible modules when they're installed.

Looks like the setup of the demo project needs some extra work. And so probably will the making-compatible-with-newer-versions-of-Django/Python-3.x. The first step needed is merging PR #42. @samluescher Any chance?

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.

None yet

3 participants