zsiciarz / django-markitup

Markup handling (using Jay Salvat's MarkItUp! flexible universal markup editor) for Django
https://github.com/zsiciarz/django-markitup
BSD 3-Clause "New" or "Revised" License
47 stars 31 forks source link

Fixed ``TypeError: Item in ``from list'' not a string`` error in Python 2 #5

Closed berkerpeksag closed 10 years ago

berkerpeksag commented 10 years ago

Sorry, I broke django-markitup in Python 2 :(

carljm commented 10 years ago

Thanks for catching and fixing. Can you also add a test here that fails under Python 2 without this fix, and passes with it? We both relied on the tests to confirm that you hadn't broken anything, and clearly the test coverage was inadequate in this area, so let's fix that while we're at it.

berkerpeksag commented 10 years ago

I think actual the problem is in usage of __import__. My MARKITUP_FILTER setting is:

MARKITUP_FILTER = ('markdown.markdown', {'safe_mode': True})

With current code, the __import__ call is __import__(u'markdown', {}, {}, [u'markdown']), but the second markdown is not a module object.

The __import__ implementation of Python is ignoring fromlist argument if it couldn't find a module object (of course fromlist argument should be str in Python 2):

>>> __import__(u'markdown', {}, {}, ['markdown'])
<module 'markdown' from '/home/berker/hacking/django-markitup/venv/local/lib/python2.7/site-packages/markdown/__init__.pyc'>

Could you review the new patch?

Can you also add a test here that fails under Python 2 without this fix, and passes with it?

I can do that, but how can I override the MARKITUP_FILTER setting in tests?

carljm commented 10 years ago

You can override settings manually (as is done in some of the existing tests) using setUp and tearDown or try/finally, or you can use django.test.utils.override_settings (which didn't exist yet when django-markitup was written, but is now present in all supported versions of Django).

carljm commented 10 years ago

Reviewing the new patch, I don't think it's correct. There's no reason the port to Python 3 should require a change in whether fromlist is empty or not. I would guess the problem is simply a matter of trying to pass a unicode object instead of a string in fromlist (in Python 2), so the encode under Python 2 should be all that is needed, the if/else looks wrong to me.

carljm commented 10 years ago

Merged a fix from Neil Muller in https://bitbucket.org/carljm/django-markitup/pull-request/13/fix-typeerror-item-in-from-list-not-a/diff