Merge lp:~brian.curtin/ubuntu-sso-client/py3-unicode into lp:ubuntu-sso-client

[Brian Curtin]

The first of several changes to prepare sso for Python 3 Unicode usage

The first of several branches which prepare our code to work with both Python 2 and 3 with respect to Unicode. Since we support Python 2.6 and beyond as well as Python 3, we make use of the "from __future__ import unicode_literals" compile change, which makes things a lot easier.

In addition, I introduced ubuntu_sso.utils.compat (at the very end of the diff) which is the start of the minimal compatibility layer we'll need to maintain between Python 2 and 3. It's a fairly common approach, and as stated in the accompanying comment, it's outlined at http://python3porting.com/noconv.html#more-bytes-strings-and-unicode

(compat may make sense to move into something like devtools at a later time. I figure it's best to keep small changes self-contained for the time being before we do any cross-project changes)

This change covers modules in the following packages:
ubuntu_sso
ubuntu_sso.gtk
ubuntu_sso.keyring
ubuntu_sso.main

[Alejandro J. Cura (alecu)]

This series of py3k branches is looking awesome, thanks for working on it!

One thing we should consider: in every file where we add the "from __future__ import unicode_literals" we should be extra careful about the string literals that we are not turning from u"..." to "...".

I can see two different cases happening here:
 * In some places the old code assumes that conversions when comparing unicode with str are automatic, so to make this work on 3 we should leave old string literal as they are.
 * In some other places some old string literals should be changed from "..." to "...".encode(...), because they are truly using "byte literals". For instance in many of the tests in ubuntu_sso/keyring/tests/test_common.py that fake some functions that will still return bytes on py3.

review: Needs Fixing

[Brian Curtin (brian.curtin)]

I think the above changes should have this addressed. Rather than changing the tests, I made the implementation more Python 3 safe. keyring.gethostname actually would not work on 3 anyway.

[Alejandro J. Cura (alecu)]

== Python Lint Notices ==

ubuntu_sso/qt/__init__.py:
    50: [W0511] TODO

ubuntu_sso/utils/compat.py:
    42: [W0622] Redefining built-in 'basestring'
    40: [C0103] Invalid name "text_type" (should match (([A-Z_][A-Z0-9_]*)|(__.*__))$)
    41: [C0103] Invalid name "binary_type" (should match (([A-Z_][A-Z0-9_]*)|(__.*__))$)
    42: [C0103] Invalid name "basestring" (should match (([A-Z_][A-Z0-9_]*)|(__.*__))$)
    44: [C0103] Invalid name "text_type" (should match (([A-Z_][A-Z0-9_]*)|(__.*__))$)
    45: [C0103] Invalid name "binary_type" (should match (([A-Z_][A-Z0-9_]*)|(__.*__))$)
    46: [C0103] Invalid name "basestring" (should match (([A-Z_][A-Z0-9_]*)|(__.*__))$)

ubuntu_sso/utils/webclient/common.py:
    286: [W0511] TODO

[Alejandro J. Cura (alecu)]

review: Needs Fixing

[Alejandro J. Cura (alecu)]

Other than lint issues, I've run the tests on Ubuntu, and ran the sso client irl, and it seems to be working fine.

[lp:~brian.curtin/ubuntu-sso-client/py3-unicode]

998. By Brian Curtin on 2012-07-24

Disable lint warnings for basestring redefine, invalid names

[Alejandro J. Cura (alecu)]

Looks good!

review: Approve

[Mike McCracken (mikemc)]

Looks good to me.

review: Approve

=== modified file 'ubuntu_sso/account.py'
--- ubuntu_sso/account.py	2012-06-22 21:04:49 +0000
+++ ubuntu_sso/account.py	2012-07-24 20:55:21 +0000
@@ -35,19 +35,21 @@
 
 """
 
+from __future__ import unicode_literals
+
 import os
 import re
 
 from twisted.internet import defer
 
 from ubuntu_sso.logger import setup_logging
-from ubuntu_sso.utils import webclient
+from ubuntu_sso.utils import compat, webclient
 from ubuntu_sso.utils.webclient import restful
 from ubuntu_sso.utils.webclient.common import WebClientError
 
 
 logger = setup_logging("ubuntu_sso.account")
-SERVICE_URL = u"https://login.ubuntu.com/api/1.0/"
+SERVICE_URL = "https://login.ubuntu.com/api/1.0/"
 SSO_STATUS_OK = 'ok'
 SSO_STATUS_ERROR = 'error'
 
@@ -112,7 +114,7 @@
         result = {}
         for key, val in errdict.items():
             # workaround until bug #624955 is solved
-            if isinstance(val, basestring):
+            if isinstance(val, compat.basestring):
                 result[key] = val
             else:
                 result[key] = "\n".join(val)
@@ -125,7 +127,7 @@
                      filename)
         restful_client = restful.RestfulClient(self.service_url)
         try:
-            captcha = yield restful_client.restcall(u"captchas.new")
+            captcha = yield restful_client.restcall("captchas.new")
         finally:
             restful_client.shutdown()
 
@@ -162,7 +164,7 @@
                 logger.error('register_user: InvalidPasswordError')
                 raise InvalidPasswordError()
 
-            result = yield restful_client.restcall(u"registration.register",
+            result = yield restful_client.restcall("registration.register",
                         email=email, password=password,
                         displayname=displayname,
                         captcha_id=captcha_id,
@@ -189,7 +191,7 @@
                                                password=password)
         try:
             credentials = yield restful_client.restcall(
-                        u"authentications.authenticate",
+                        "authentications.authenticate",
                         token_name=token_name)
         except WebClientError:
             logger.exception('login failed with:')
@@ -208,7 +210,7 @@
         restful_client = restful.RestfulClient(self.service_url,
                                                oauth_credentials=token)
         try:
-            me_info = yield restful_client.restcall(u"accounts.me")
+            me_info = yield restful_client.restcall("accounts.me")
         finally:
             restful_client.shutdown()
         key = 'preferred_email'
@@ -229,7 +231,7 @@
         restful_client = restful.RestfulClient(self.service_url,
                                                oauth_credentials=credentials)
         try:
-            result = yield restful_client.restcall(u"accounts.validate_email",
+            result = yield restful_client.restcall("accounts.validate_email",
                                                    email_token=email_token)
         finally:
             restful_client.shutdown()
@@ -247,7 +249,7 @@
         """Request a token to reset the password for the account 'email'."""
         restful_client = restful.RestfulClient(self.service_url)
         try:
-            operation = u"registration.request_password_reset_token"
+            operation = "registration.request_password_reset_token"
             result = yield restful_client.restcall(operation, email=email)
         except WebClientError as e:
             logger.exception('request_password_reset_token failed with:')
@@ -272,7 +274,7 @@
         restful_client = restful.RestfulClient(self.service_url)
         try:
             result = yield restful_client.restcall(
-                                        u"registration.set_new_password",
+                                        "registration.set_new_password",
                                         email=email, token=token,
                                         new_password=new_password)
         except WebClientError as e:

=== modified file 'ubuntu_sso/credentials.py'
--- ubuntu_sso/credentials.py	2012-07-13 21:07:38 +0000
+++ ubuntu_sso/credentials.py	2012-07-24 20:55:21 +0000
@@ -54,7 +54,7 @@
 )
 from ubuntu_sso.keyring import Keyring
 from ubuntu_sso.logger import setup_logging
-from ubuntu_sso.utils import get_bin_cmd, runner
+from ubuntu_sso.utils import compat, get_bin_cmd, runner
 
 
 logger = setup_logging('ubuntu_sso.credentials')
@@ -189,8 +189,8 @@
             value = getattr(self, arg)
             if value:
                 args.append('--%s' % arg)
-                if not isinstance(value, basestring):
-                    value = str(value)
+                if not isinstance(value, compat.basestring):
+                    value = compat.text_type(value)
                 args.append(value)
 
         if login_only:

=== modified file 'ubuntu_sso/keyring/__init__.py'
--- ubuntu_sso/keyring/__init__.py	2012-06-27 20:41:19 +0000
+++ ubuntu_sso/keyring/__init__.py	2012-07-24 20:55:21 +0000
@@ -28,6 +28,8 @@
 # files in the program, then also delete it here.
 """Implementations of different keyrings."""
 
+from __future__ import unicode_literals
+
 import socket
 import sys
 
@@ -41,11 +43,12 @@
 from twisted.internet.defer import inlineCallbacks, returnValue
 
 from ubuntu_sso.logger import setup_logging
+from ubuntu_sso.utils import compat
 
 logger = setup_logging("ubuntu_sso.keyring")
 
-TOKEN_SEPARATOR = u' @ '
-SEPARATOR_REPLACEMENT = u' AT '
+TOKEN_SEPARATOR = ' @ '
+SEPARATOR_REPLACEMENT = ' AT '
 
 U1_APP_NAME = "Ubuntu One"
 U1_KEY_NAME = "UbuntuOne token for https://ubuntuone.com"
@@ -58,7 +61,9 @@
 def gethostname():
     """Get the hostname, return the name as unicode."""
     sys_encoding = sys.getfilesystemencoding()
-    hostname = socket.gethostname().decode(sys_encoding)
+    hostname = socket.gethostname()
+    if isinstance(hostname, compat.binary_type):
+        return hostname.decode(sys_encoding)
     return hostname
 
 
@@ -68,10 +73,10 @@
     computer_name = gethostname()
     quoted_computer_name = quote(computer_name)
 
-    assert isinstance(computer_name, unicode)
-    assert isinstance(quoted_computer_name, unicode)
+    assert isinstance(computer_name, compat.text_type)
+    assert isinstance(quoted_computer_name, compat.text_type)
 
-    return u"%s - %s" % (quoted_app_name, quoted_computer_name)
+    return "%s - %s" % (quoted_app_name, quoted_computer_name)
 
 
 def get_token_name(app_name):
@@ -80,8 +85,8 @@
     computer_name = computer_name.replace(TOKEN_SEPARATOR,
                                           SEPARATOR_REPLACEMENT)
 
-    assert isinstance(computer_name, unicode)
-    assert isinstance(computer_name, unicode)
+    assert isinstance(computer_name, compat.text_type)
+    assert isinstance(computer_name, compat.text_type)
 
     return TOKEN_SEPARATOR.join((app_name, computer_name))
 

=== modified file 'ubuntu_sso/keyring/tests/test_common.py'
--- ubuntu_sso/keyring/tests/test_common.py	2012-04-09 17:38:24 +0000
+++ ubuntu_sso/keyring/tests/test_common.py	2012-07-24 20:55:21 +0000
@@ -33,6 +33,8 @@
 # files in the program, then also delete it here.
 """Tests for the keyring common module."""
 
+from __future__ import unicode_literals
+
 from twisted.trial.unittest import TestCase
 
 from ubuntu_sso import keyring
@@ -49,7 +51,7 @@
 
     def test_get_hostname_uses_filesystem_encoding(self):
         """The fs encoding is used to decode the name returned by socket."""
-        fake_hostname = u"Привет-ПК"
+        fake_hostname = "Привет-ПК"
         hostname_koi8r = fake_hostname.encode("koi8-r")
         self.patch(keyring.socket, "gethostname", lambda: hostname_koi8r)
         self.patch(keyring.sys, "getfilesystemencoding", lambda: "koi8-r")
@@ -67,35 +69,35 @@
 
     def test_get_simple_token_name(self):
         """A simple token name is built right."""
-        sample_app_name = u"UbuntuTwo"
+        sample_app_name = "UbuntuTwo"
         sample_hostname = "Darkstar"
         expected_result = "UbuntuTwo @ Darkstar"
         self.check_build(sample_app_name, sample_hostname, expected_result)
 
     def test_get_complex_token_name_for_app_name(self):
         """A complex token name is built right too."""
-        sample_app_name = u"Ubuntu @ Eleven"
+        sample_app_name = "Ubuntu @ Eleven"
         sample_hostname = "Mate+Cocido"
         expected_result = "Ubuntu @ Eleven @ Mate+Cocido"
         self.check_build(sample_app_name, sample_hostname, expected_result)
 
     def test_get_complex_token_name_for_hostname(self):
         """A complex token name is built right too."""
-        sample_app_name = u"Ubuntu Eleven"
+        sample_app_name = "Ubuntu Eleven"
         sample_hostname = "Mate @ Cocido"
         expected_result = "Ubuntu Eleven @ Mate AT Cocido"
         self.check_build(sample_app_name, sample_hostname, expected_result)
 
     def test_get_unicode_appname_token_name(self):
         """A token name with unicode in the app name."""
-        sample_app_name = u"Ubuntu 四百六十九"
+        sample_app_name = "Ubuntu 四百六十九"
         sample_hostname = "Darkstar"
-        expected_result = u"Ubuntu 四百六十九 @ Darkstar"
+        expected_result = "Ubuntu 四百六十九 @ Darkstar"
         self.check_build(sample_app_name, sample_hostname, expected_result)
 
     def test_get_utf8_hostname_token_name(self):
         """A token name with utf8 in the host name."""
-        sample_app_name = u"Ubuntu Eleven"
-        sample_hostname = u"Привет-ПК"
-        expected_result = u"Ubuntu Eleven @ Привет-ПК"
+        sample_app_name = "Ubuntu Eleven"
+        sample_hostname = "Привет-ПК"
+        expected_result = "Ubuntu Eleven @ Привет-ПК"
         self.check_build(sample_app_name, sample_hostname, expected_result)

=== modified file 'ubuntu_sso/keyring/tests/test_linux.py'
--- ubuntu_sso/keyring/tests/test_linux.py	2012-06-22 16:55:35 +0000
+++ ubuntu_sso/keyring/tests/test_linux.py	2012-07-24 20:55:21 +0000
@@ -33,6 +33,8 @@
 # files in the program, then also delete it here.
 """Tests for the keyring.py module."""
 
+from __future__ import unicode_literals
+
 from twisted.internet import defer
 from twisted.internet.defer import inlineCallbacks
 from twisted.trial.unittest import TestCase
@@ -133,7 +135,7 @@
         self.mock_service = None
         self.service = self.patch(keyring, "SecretService",
                                   self.get_mock_service)
-        self.patch(common_keyring, "gethostname", lambda: u"darkstar")
+        self.patch(common_keyring, "gethostname", lambda: "darkstar")
 
     def get_mock_service(self):
         """Create only one instance of the mock service per test."""

=== modified file 'ubuntu_sso/main/__init__.py'
--- ubuntu_sso/main/__init__.py	2012-07-12 15:54:13 +0000
+++ ubuntu_sso/main/__init__.py	2012-07-24 20:55:21 +0000
@@ -52,6 +52,7 @@
 )
 from ubuntu_sso.keyring import get_token_name, Keyring
 from ubuntu_sso.logger import setup_logging, log_call
+from ubuntu_sso.utils import compat
 
 
 logger = setup_logging("ubuntu_sso.main")
@@ -109,7 +110,7 @@
         result["message"] = e.__class__.__doc__
     elif isinstance(e.args[0], dict):
         result.update(e.args[0])
-    elif isinstance(e.args[0], basestring):
+    elif isinstance(e.args[0], compat.basestring):
         result["message"] = e.args[0]
 
     return result

=== modified file 'ubuntu_sso/main/tests/test_common.py'
--- ubuntu_sso/main/tests/test_common.py	2012-04-09 17:38:24 +0000
+++ ubuntu_sso/main/tests/test_common.py	2012-07-24 20:55:21 +0000
@@ -28,6 +28,8 @@
 # files in the program, then also delete it here.
 """Tests for the main SSO client code."""
 
+from __future__ import unicode_literals
+
 import logging
 
 from functools import partial
@@ -137,7 +139,7 @@
         class MyException(Exception):
             """Custom Exception."""
 
-        message = u'My custom error for ♥ Ubuntu'
+        message = 'My custom error for ♥ Ubuntu'
         my_exc = MyException(message)
         result = except_to_errdict(my_exc)
         self.assertEqual(result["errtype"], my_exc.__class__.__name__)
@@ -145,7 +147,7 @@
 
     def test_first_arg_is_unicode(self):
         """If the first arg is a unicode, use it as the message."""
-        sample_string = u"a sample string"
+        sample_string = "a sample string"
         e = TestExceptToErrdictException(sample_string)
         result = except_to_errdict(e)
         self.assertEqual(result["errtype"], e.__class__.__name__)
@@ -160,7 +162,7 @@
 
     def test_some_other_thing_as_first_arg(self):
         """If first arg is not basestring nor dict, then repr all args."""
-        sample_args = (None, u"unicode2\ufffd", "errorcode3")
+        sample_args = (None, "unicode2\ufffd", "errorcode3")
         e = TestExceptToErrdictException(*sample_args)
         result = except_to_errdict(e)
         self.assertEqual(result["errtype"], e.__class__.__name__)

=== modified file 'ubuntu_sso/tests/test_account.py'
--- ubuntu_sso/tests/test_account.py	2012-07-03 21:04:09 +0000
+++ ubuntu_sso/tests/test_account.py	2012-07-24 20:55:21 +0000
@@ -31,6 +31,8 @@
 # files in the program, then also delete it here.
 """Tests for the SSO account code."""
 
+from __future__ import unicode_literals
+
 import os
 
 from twisted.trial.unittest import TestCase
@@ -69,7 +71,7 @@
 CANT_RESET_PASSWORD_CONTENT = "CanNotResetPassowrdError: " \
                               "Can't reset password for this account"
 RESET_TOKEN_INVALID_CONTENT = "AuthToken matching query does not exist."
-EMAIL_ALREADY_REGISTERED = u'[e-mail]'
+EMAIL_ALREADY_REGISTERED = '[e-mail]'
 STATUS_UNKNOWN = {'status': 'yadda-yadda'}
 STATUS_ERROR = {'status': SSO_STATUS_ERROR,
                 'errors': {'something': ['Bla', 'Ble']}}
@@ -177,12 +179,12 @@
 
     def fake_accounts_me(self):
         """Fake the 'me' information."""
-        return {u'username': u'Wh46bKY',
-                u'preferred_email': self.preferred_email,
-                u'displayname': u'',
-                u'unverified_emails': [u'[e-mail]'],
-                u'verified_emails': [],
-                u'openid_identifier': u'Wh46bKY'}
+        return {'username': 'Wh46bKY',
+                'preferred_email': self.preferred_email,
+                'displayname': '',
+                'unverified_emails': ['[e-mail]'],
+                'verified_emails': [],
+                'openid_identifier': 'Wh46bKY'}
 
     def check_all_kwargs_unicode(self, **kwargs):
         """Check that the values of all keyword arguments are unicode."""
@@ -254,27 +256,27 @@
     @defer.inlineCallbacks
     def test_register_user_checks_valid_email(self):
         """Email is validated."""
-        self.register_kwargs['email'] = u'notavalidemail'
+        self.register_kwargs['email'] = 'notavalidemail'
         d = self.processor.register_user(**self.register_kwargs)
         yield self.assertFailure(d, InvalidEmailError)
 
     @defer.inlineCallbacks
     def test_register_user_checks_valid_password(self):
         """Password is validated."""
-        self.register_kwargs['password'] = u''
+        self.register_kwargs['password'] = ''
         d = self.processor.register_user(**self.register_kwargs)
         yield self.assertFailure(d, InvalidPasswordError)
 
         # 7 chars, one less than expected
-        self.register_kwargs['password'] = u'tesT3it'
-        d = self.processor.register_user(**self.register_kwargs)
-        yield self.assertFailure(d, InvalidPasswordError)
-
-        self.register_kwargs['password'] = u'test3it!'  # no upper case
-        d = self.processor.register_user(**self.register_kwargs)
-        yield self.assertFailure(d, InvalidPasswordError)
-
-        self.register_kwargs['password'] = u'testIt!!'  # no number
+        self.register_kwargs['password'] = 'tesT3it'
+        d = self.processor.register_user(**self.register_kwargs)
+        yield self.assertFailure(d, InvalidPasswordError)
+
+        self.register_kwargs['password'] = 'test3it!'  # no upper case
+        d = self.processor.register_user(**self.register_kwargs)
+        yield self.assertFailure(d, InvalidPasswordError)
+
+        self.register_kwargs['password'] = 'testIt!!'  # no number
         d = self.processor.register_user(**self.register_kwargs)
         yield self.assertFailure(d, InvalidPasswordError)
 

=== added file 'ubuntu_sso/utils/compat.py'
--- ubuntu_sso/utils/compat.py	1970-01-01 00:00:00 +0000
+++ ubuntu_sso/utils/compat.py	2012-07-24 20:55:21 +0000
@@ -0,0 +1,49 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright 2012 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 3, as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# In addition, as a special exception, the copyright holders give
+# permission to link the code of portions of this program with the
+# OpenSSL library under certain conditions as described in each
+# individual source file, and distribute linked combinations
+# including the two.
+# You must obey the GNU General Public License in all respects
+# for all of the code used other than OpenSSL.  If you modify
+# file(s) with this exception, you may extend this exception to your
+# version of the file(s), but you are not obligated to do so.  If you
+# do not wish to do so, delete this exception statement from your
+# version.  If you delete this exception statement from all source
+# files in the program, then also delete it here.
+"""Python 2 and 3 compatibility."""
+
+from __future__ import unicode_literals
+
+# The following approach was outlined in Lennart Regebro's
+# "Porting to Python 3" book.
+# http://python3porting.com/noconv.html#more-bytes-strings-and-unicode
+
+import sys
+
+# Disable redefined builtin, invalid name warning
+# pylint: disable=W0622,C0103
+
+if sys.version_info < (3,):
+    text_type = unicode
+    binary_type = str
+    basestring = basestring
+else:
+    text_type = str
+    binary_type = bytes
+    basestring = str