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

Enhanced unicode support for models #3368

Closed
wants to merge 9 commits into from
Closed

Enhanced unicode support for models #3368

wants to merge 9 commits into from

Conversation

mmarzantowicz
Copy link

Just the beginning of long way to fully support PY3 and PY2 regarding strings, unicode etc.

  1. Use from __future__ import unicode_literals to inform python 2.x that we like it to behave like python 3.x
  2. Use django's version of python_2_unicode_compatible (it is present in django 1.4 and higher)
  3. Use django's version of force_text which is equivalent to force_unicode in some cases
  4. Drop u'...' convention for marking strings as unicode (refer to point 1)
  5. Make some minor style changes and cleanups.

Stephan Herzog and others added 3 commits August 7, 2014 20:32
I added a warning about returning non-utf8-encoded strings from within ``def __unicode__()``, as I found it resulting in a 500 response and a plugin instance marked as <Empty>. After returning as UTF8 everything worked as expected. If you can't confirm these issues to be related or feel that the addition doesn't make sense for documentation readers, feel free to drop it.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.17%) when pulling 610240f on mmarzantowicz:feature/python_2_unicode_compatible into 0326088 on divio:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.18%) when pulling 610240f on mmarzantowicz:feature/python_2_unicode_compatible into 0326088 on divio:develop.

Allow using django CMS with custom admin site
@mmarzantowicz
Copy link
Author

Reasons for failed test are strictly technical issues:

  1. github.com[0: 192.30.252.131]: errno=Connection timed out
  2. No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

@yakky
Copy link
Member

yakky commented Aug 11, 2014

Tests restarted.
Please rebase against latest develop, though.

digi604 and others added 3 commits August 11, 2014 10:22
…ble' into feature/python_2_unicode_compatible

Conflicts:
	cms/models/placeholdermodel.py
	cms/models/pluginmodel.py
@digi604
Copy link
Contributor

digi604 commented Aug 11, 2014

still failing?

@beniwohli
Copy link
Contributor

I would be cautious about introducing from __future__ import unicode_literals into the code base. There are lots of good arguments from smart people that speak against its usage. For a discussion, see

@mmarzantowicz
Copy link
Author

@piquadrat, do you have anything particular against from __future__ import unicode_literals in this PR? Where does it break anything or make it worse? If django CMS decides not to go the django way, do you have any better idea on how to support py2-py3 transition?

@mmarzantowicz
Copy link
Author

The test failed at No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.. Otherwise it's now clean.

@mmarzantowicz
Copy link
Author

@piquadrat, I've carefully read discussions you pasted about unicode_literals. Thank you for signaling potential issues. I will try to avoid as much of them as possible. If you think django CMS should not rely on unicode_literals, we should decide on other solution because doing nothing is not an option.

@coveralls
Copy link

coveralls commented Aug 12, 2014

Coverage Status

Coverage increased (+0.05%) to 87.365% when pulling 610240f on mmarzantowicz:feature/python_2_unicode_compatible into 0326088 on divio:develop.

@beniwohli
Copy link
Contributor

@mmarzantowicz I don't understand why unicode_literals is needed in the first place? Why not just continue using u''?

@mmarzantowicz
Copy link
Author

Generally speaking unicode_literals makes code to behave in py2 like it behaves in py3 (regarding strings representation.) Django uses this (and recommends) in conjunction to six library to allow py2 and py3 simultaneously. Now, why should we use unicode_literals? To avoid using u'' for example. Maybe for you it's OK to prefix each literal with u'' and technically you're right (syntax allows it) but it's really ugly (that is my opinion.) What is more in py3 you don't need to use this prefix. Since we're aiming at py3 and dropping py2 at some time (year 2020 as stated by Guido) we should make our code be written for py3 and support py2 for some transitional time. After py2 finally dies we could remove ONE line from each file.

Please also take into account that I'm only touching models in this PR where unicode_literals might simplify things. I agree that it's not vise or required to use this trick everywhere. But here, it might only help.

@yakky
Copy link
Member

yakky commented Oct 24, 2014

@mmarzantowicz still interested in this?
Now that we're moving in 3.1 we might rethink this approach as we're more likely to spot potential issues before release.
To be clear: I'm +0 on this, but if you're still interested / able to rebase it on top of develop we can discuss this
Ping @mkoistinen @evildmp @digi604 @ojii for further input

@timgraham
Copy link
Contributor

I am biased from having worked on Django so much, but the unicode_literals approach seems cleaner than requiring explicit u'' or b'' strings everywhere. Yes, there will likely be some bugs as Django had, but it's a good opportunity to increase the coverage of your test suite as you fix them. When Python 2 reaches end-of-life, you can simply drop unicode_literals and avoid the ugliness of u'' prefixes. In this respect, as Aymeric suggested in one of the docs linked above, I think of Django as a Python 3 project with legacy support for Python 2 at this point.

@yakky yakky added this to the 3.1 milestone Dec 21, 2014
@yakky
Copy link
Member

yakky commented Dec 21, 2014

I'd resume this
@mmarzantowicz : still interested in this?

@mmarzantowicz
Copy link
Author

Yes, I'm still interested in this topic. Please give me some time to update my repos and see what needs to be done here. Then I'll open some issues to discuss this matter deeply.

Finally, do we go "the Django way" with unicode support for models? That means, the way I presented it in my first post with optional modifications?

@yakky
Copy link
Member

yakky commented Dec 27, 2014

Consistency with upstream looks like a clear bonus to me.
Wait a bit before resuming work on this as we'll still forwardport code from 3.0 branch to develop which will break your refactoring

@mmarzantowicz mmarzantowicz deleted the feature/python_2_unicode_compatible branch January 13, 2015 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants