Merge lp:~elopio/osqa-sst-helpers/data into lp:~online-services-qa/osqa-sst-helpers/canonical-identity-provider

[Leo Arias]

Refactored data to remove the factory.

Refactored the data to remove the factory.

[Leo Arias (elopio)]

Vincent, why do you name the parameter klass?

[Vincent Ladeuil (vila)]

> Vincent, why do you name the parameter klass?

It's an idiom chosen because 'class' is a python reserved keyword so it can't be used. 'klass' is unambiguous and close enough.

[Vincent Ladeuil (vila)]

Some naming remarks:

50 + def make_for_test(klass, new_user=False):

Well, we know it's for test since the whole class is for tests.

53 + test_user = klass._make_random_test_user_from_configuration()

55 + test_user = klass._make_test_user_from_configuration()

Since all use cases are from configuration, I'd rather name this method 'make_from_configuration'.

58 + @classmethod
59 + def _make_random_test_user_from_configuration(klass):
60 + random_user_uuid = str(uuid.uuid1())
61 + full_name = 'Test user ' + random_user_uuid
62 + email_address = settings.EMAIL_ADDRESS_PATTERN % random_user_uuid
63 + password = settings.SSO_TEST_ACCOUNT_PASSWORD
64 + return klass(full_name, email_address, password)
65 +
66 + @classmethod
67 + def _make_test_user_from_configuration(klass):
68 + full_name = settings.SSO_TEST_ACCOUNT_FULL_NAME
69 + email_address = settings.SSO_TEST_ACCOUNT_EMAIL
70 + password = settings.SSO_TEST_ACCOUNT_PASSWORD
71 + return klass(full_name, email_address, password)

I think having these methods hurt readability and make for more duplication:

49 + @classmethod
58 + @classmethod
66 + @classmethod

63 + password = settings.SSO_TEST_ACCOUNT_PASSWORD
64 + return klass(full_name, email_address, password)

70 + password = settings.SSO_TEST_ACCOUNT_PASSWORD
71 + return klass(full_name, email_address, password)

So I'd rather go with these methods inlined in
'make_from_configuration'. This way the reader can focus on the still short
enough 'make_from_configuration'.

An additional trap in your implementation is that the two methods are not in
the order they are used (so, as a reader, if I don't pay attention enough I
won't notice and read the wrong method when reading 'make_for_test').

'_make_random_test_user_from_configuration' and
'_make_test_user_from_configuration' being this long (note that I can't cite
them both on a 80 chars long line), it's hard to spot the differing
'random', that's, for me, an example of why names too long hurt readability
instead of helping it. Rmeebmer that popele are good at recgniozing words
based on the first and last letetr even if the letters inside the words are
suhffled (you managed to read this sentence without too much trouble aren't
you ? ;).

I.e.:

....

wth, I cannot branch lp:osqa-sst-helpers ? What is this project about ? 8-/
(that was rhetorical, the development focus needs to be set in lp).

anyway:

=== modified file 'sso_sst_helpers/data.py'
--- sso_sst_helpers/data.py 2012-10-10 21:07:11 +0000
+++ sso_sst_helpers/data.py 2012-10-11 07:47:48 +0000
@@ -30,25 +30,13 @@
         return '%s(%r)' % (self.__class__, self.__dict__)

     @classmethod
- def make_for_test(klass, new_user=False):
+ def make_from_configuration(klass, new_user=False):
         test_user = None
         if new_user:
- test_user = klass._make_random_test_user_from_configuration()
+ full_name = settings.SSO_TEST_ACCOUNT_FULL_NAME
+ email_address = settings.SSO_TEST_ACCOUNT_EMAIL
         else:
- test_user = klass._make_test_user_from_configuration()
- return test_user
-
- @classmethod
- def _make_random_test_user_from_configuration(klass):
- random_u...

review: Needs Fixing

[lp:~elopio/osqa-sst-helpers/data]

39. By Leo Arias on 2012-10-11

Added the docstring.
Renamed the factory function to make_from_configuration.
Refactored following Vincent's suggetions.

40. By Leo Arias on 2012-10-11

Fixed the comments identation.

[Vincent Ladeuil (vila)]

Excellent !

There are some conventions for formatting docstrings but that can wait (and I'll have to search the reference anyway ;)

review: Approve

[Leo Arias (elopio)]

For docstrings: http://www.python.org/dev/peps/pep-0257/

[lp:~elopio/osqa-sst-helpers/data]

41. By Leo Arias on 2012-10-16

Merged with trunk.

42. By Leo Arias on 2012-10-16

Fixed style error.

=== modified file 'setup.py'
--- setup.py	2012-09-18 01:06:17 +0000
+++ setup.py	2012-10-16 16:49:18 +0000
@@ -28,7 +28,6 @@
 PACKAGES = [
     'sso_sst_helpers', 
     'sso_sst_helpers.actions', 
-    'sso_sst_helpers.data',
     'sso_sst_helpers.utils'
 ]
 

=== removed directory 'sso_sst_helpers/data'
=== added file 'sso_sst_helpers/data.py'
--- sso_sst_helpers/data.py	1970-01-01 00:00:00 +0000
+++ sso_sst_helpers/data.py	2012-10-16 16:49:18 +0000
@@ -0,0 +1,56 @@
+# -*- 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 Lesser 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 Lesser 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/>.
+
+import uuid
+
+from django.conf import settings
+
+
+class User(object):
+
+    def __init__(self, full_name, email, password):
+        self.full_name = full_name
+        self.email = email
+        self.password = password
+
+    def __repr__(self):
+        return '%s(%r)' % (self.__class__, self.__dict__)
+
+    @classmethod
+    def make_from_configuration(cls, new_user=False):
+        """Return a user taking its credentials from the configuration files.
+
+        Keyword arguments:
+        new_user -- If True, the full name and email of the user will have a
+            UUID to make them practically unique. In this case, the email will
+            be formed with the value of the EMAIL_ADDRESS_PATTERN
+            configuration variable. If False, the full name and email will be
+            the values of the SSO_TEST_ACCOUNT_FULL_NAME and
+            SSO_TEST_ACCOUNT_EMAIL configuration variables, respectively. In
+            both cases, the password will be the value of the
+            SSO_TEST_ACCOUNT_PASSWORD configuration variable.
+            Default is False.
+
+        """
+        if new_user:
+            random_user_uuid = str(uuid.uuid1())
+            full_name = 'Test user ' + random_user_uuid
+            email = settings.EMAIL_ADDRESS_PATTERN % random_user_uuid
+        else:
+            full_name = settings.SSO_TEST_ACCOUNT_FULL_NAME
+            email = settings.SSO_TEST_ACCOUNT_EMAIL
+        password = settings.SSO_TEST_ACCOUNT_PASSWORD
+        return cls(full_name, email, password)

=== removed file 'sso_sst_helpers/data/__init__.py'
=== removed file 'sso_sst_helpers/data/factory.py'
--- sso_sst_helpers/data/factory.py	2012-10-15 20:38:20 +0000
+++ sso_sst_helpers/data/factory.py	1970-01-01 00:00:00 +0000
@@ -1,45 +0,0 @@
-# -*- 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 Lesser 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 Lesser 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/>.
-
-import uuid
-
-from django.conf import settings
-
-from sso_sst_helpers.data.user import User
-
-
-def make_test_user(new_user=False):
-    test_user = None
-    if new_user:
-        test_user = _make_random_test_user_from_configuration()
-    else:
-        test_user = _make_test_user_from_configuration()
-    return test_user
-
-
-def _make_random_test_user_from_configuration():
-    random_user_uuid = str(uuid.uuid1())
-    full_name = 'Test user ' + random_user_uuid
-    email_address = settings.EMAIL_ADDRESS_PATTERN % random_user_uuid
-    password = settings.SSO_TEST_ACCOUNT_PASSWORD
-    return User(full_name, email_address, password)
-
-
-def _make_test_user_from_configuration():
-    full_name = settings.SSO_TEST_ACCOUNT_FULL_NAME
-    email_address = settings.SSO_TEST_ACCOUNT_EMAIL
-    password = settings.SSO_TEST_ACCOUNT_PASSWORD
-    return User(full_name, email_address, password)

=== removed file 'sso_sst_helpers/data/user.py'
--- sso_sst_helpers/data/user.py	2012-10-15 20:38:20 +0000
+++ sso_sst_helpers/data/user.py	1970-01-01 00:00:00 +0000
@@ -1,23 +0,0 @@
-# -*- 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 Lesser 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 Lesser 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/>.
-
-
-class User:
-
-    def __init__(self, full_name, email, password):
-        self.full_name = full_name
-        self.email = email
-        self.password = password