Merge lp:~salgado/offspring/object-factory into lp:offspring

[Guilherme Salgado]

Create a new factory of (django) model objects to be used in tests

[Kevin McDermott (bigkevmcd)]

#1

+ user = User.objects.create_user(
+ userid, self.get_unique_email_address(), password)
+ user.save()
+ return user

This could be simplified to

return User.objects.create_user(...)

The documentation for .create... "A convenience method for creating an object and saving it all in one step"

#2

You alternate between objects.create and direct instantiation throughout the code, any particular reason why?

#3

Can we aim for PEP8 compliance? i.e. "Function names should be lowercase, with words separated by underscores as necessary to improve readability."

def make_projectGrou should be def make_project_group

#4
You've added docstrings to some methods, but not others, can you add docstrings to each method, explaining what they do?

A headsup too...in my database-testing branch, I've removed offspring.config.config, I added get_configuration to offspring.config which returns a ConfigParser-type object, so...

    base_dir = config.master('result_directory')

will become...

    config = offspring.config.get_configuration()
    base_Dir = config.get('master', 'result_directory')

Of course, you can do...

   def __init__(...):
       ...
       self.config = offspring.config.get_configuration()

and then refer to self.config everywhere...

But, in this branch, prior to that getting landed, you can still change the config.master(...) reference to config.get('master', ...) which is what it's doing.

review: Needs Fixing

[Guilherme Salgado (salgado)]

On Tue, 2011-11-22 at 10:09 +0000, Kevin McDermott wrote:
> Review: Needs Fixing
>
> #1
>
> + user = User.objects.create_user(
> + userid, self.get_unique_email_address(), password)
> + user.save()
> + return user
>
> This could be simplified to
>
> return User.objects.create_user(...)
>
> The documentation for .create... "A convenience method for creating an
> object and saving it all in one step"

Oh, indeed; changed.

>
> #2
>
> You alternate between objects.create and direct instantiation
> throughout the code, any particular reason why?

Well, it's only for User that I use .objects.create_user(), and that's
because I think it's specific to UserManager. If I'm wrong I'd be happy
to use it for others as well.

> #3
>
> Can we aim for PEP8 compliance? i.e. "Function names should be
> lowercase, with words separated by underscores as necessary to improve
> readability."
>
> def make_projectGrou should be def make_project_group

That was an oversight when I renamed all methods to be PEP8 compliant.
I've fixed it.

>
> #4
> You've added docstrings to some methods, but not others, can you add
> docstrings to each method, explaining what they do?

TBH I didn't add any -- that one was copy-n-pasted from another
project. ;)

However, do you really think any of them need a docstring? I think
they're simple/obvious enough to not warrant one.

> A headsup too...in my database-testing branch, I've removed
> offspring.config.config, I added get_configuration to offspring.config
> which returns a ConfigParser-type object, so...
>
> base_dir = config.master('result_directory')
>
> will become...
>
> config = offspring.config.get_configuration()
> base_Dir = config.get('master', 'result_directory')
>
> Of course, you can do...
>
> def __init__(...):
> ...
> self.config = offspring.config.get_configuration()
>
> and then refer to self.config everywhere...
>
> But, in this branch, prior to that getting landed, you can still
> change the config.master(...) reference to config.get('master', ...)
> which is what it's doing.

But this branch doesn't change any code using that -- it's the previous
one (path-independence) that does. Maybe the diff you reviewed included
the changes from path-independence as well?

[lp:~salgado/offspring/object-factory]

41. By Guilherme Salgado on 2011-11-22

PEP8 compliance and do not call .save() on the return value of User.objects.create_user() as that's not necessary

[Kevin McDermott (bigkevmcd)]

> > #2
> >
> > You alternate between objects.create and direct instantiation
> > throughout the code, any particular reason why?

Yeah, but all the other managers can create, saving you the .save() so...

+ def make_build_request(self, project=None):
+ if project is None:
+ project = self.make_project()
+ project.save()
+ request = BuildRequest(project=project)
+ request.save()
+ return request

could become...

    def make_build_request(self, project=None):
        if project is None:
            project = self.make_project()
        return BuildRequest.objects.create(project=project)

> > #4
> > You've added docstrings to some methods, but not others, can you add
> > docstrings to each method, explaining what they do?

They can have a simple one, for example...
"""
Create a build_request, if no project is given, create a project and associate the build with the newly created project.
"""

Sorry, the bit about config was just a headsup, it shouldn't really have gone in there...

[lp:~salgado/offspring/object-factory]

42. By Guilherme Salgado on 2011-11-22

Add docstrings and use .objects.create() to avoid a save() call

[Guilherme Salgado (salgado)]

On Tue, 2011-11-22 at 17:18 +0000, Kevin McDermott wrote:
> > > #2
> > >
> > > You alternate between objects.create and direct instantiation
> > > throughout the code, any particular reason why?
>
> Yeah, but all the other managers can create, saving you the .save() so.

Oh, cool, I've changed them all, but I couldn't find
ModelManager.create() in the docs. Care to point me where you found
them?

> ..
>
> + def make_build_request(self, project=None):
> + if project is None:
> + project = self.make_project()
> + project.save()
> + request = BuildRequest(project=project)
> + request.save()
> + return request
>
> could become...
>
> def make_build_request(self, project=None):
> if project is None:
> project = self.make_project()
> return BuildRequest.objects.create(project=project)

Yep, much simpler.

>
> > > #4
> > > You've added docstrings to some methods, but not others, can you add
> > > docstrings to each method, explaining what they do?
>
> They can have a simple one, for example...
> """
> Create a build_request, if no project is given, create a project and associate the build with the newly created project.
> """

To me this feels like I'm repeating myself as it's not saying anything
that's not easily inferred from the method's signature, but I've added
them. :)

[Kevin McDermott (bigkevmcd)]

> Oh, cool, I've changed them all, but I couldn't find
> ModelManager.create() in the docs. Care to point me where you found
> them?

https://docs.djangoproject.com/en/1.3/ref/models/querysets/#create :-)

>
> To me this feels like I'm repeating myself as it's not saying anything
> that's not easily inferred from the method's signature, but I've added
> them. :)

Thanks :-)

Looks good to me, +1

[Kevin McDermott (bigkevmcd)]

review: Approve

=== added directory 'lib/offspring/web/queuemanager/tests'
=== added file 'lib/offspring/web/queuemanager/tests/__init__.py'
=== added file 'lib/offspring/web/queuemanager/tests/factory.py'
--- lib/offspring/web/queuemanager/tests/factory.py	1970-01-01 00:00:00 +0000
+++ lib/offspring/web/queuemanager/tests/factory.py	2011-11-22 18:30:29 +0000
@@ -0,0 +1,112 @@
+from itertools import count
+
+from django.contrib.auth.models import User
+
+from offspring.web.queuemanager.models import (
+    BuildRequest,
+    BuildResult,
+    Lexbuilder,
+    ProjectGroup,
+    Project,
+    Release)
+
+
+class ObjectFactory(object):
+
+    def __init__(self):
+        self.counter = count()
+
+    def get_unique_email_address(self):
+        """Return an email address unique to this factory instance."""
+        return "[e-mail]" % self.get_unique_string('email')
+
+    def get_unique_string(self, prefix=None):
+        """Return a string unique to this factory instance.
+
+        :param prefix: Used as a prefix for the unique string. If unspecified,
+            defaults to 'generic-string'.
+        """
+        if prefix is None:
+            prefix = "generic-string"
+        string = "%s%s" % (prefix, self.get_unique_integer())
+        return string.lower()
+
+    def get_unique_integer(self):
+        """Return an integer unique to this factory instance."""
+        return self.counter.next()
+
+    def make_user(self):
+        """Create a new User."""
+        userid = password = self.get_unique_string()
+        return User.objects.create_user(
+            userid, self.get_unique_email_address(), password)
+
+    def make_project(self, name=None, title=None):
+        """
+        Create a Project with the given name and title. If any of those is not
+        given we use an arbitrary value.
+        """
+        if name is None:
+            name = self.get_unique_string()
+        if title is None:
+            title = self.get_unique_string()
+        return Project.objects.create(name=name, title=title)
+
+    def make_project_group(self, name=None, title=None):
+        """
+        Create a ProjectGroup with the given name and title. If any of those
+        is not given we use an arbitrary value.
+        """
+        if name is None:
+            name = self.get_unique_string()
+        if title is None:
+            title = self.get_unique_string()
+        return ProjectGroup.objects.create(name=name, title=title)
+
+    def make_build_result(self, project=None):
+        """
+        Create a BuildResult with the given project or a newly created one if
+        none is given.
+        """
+        if project is None:
+            project = self.make_project()
+        return BuildResult.objects.create(project=project)
+
+    def make_release(self, build=None, name=None, creator=None):
+        """
+        Create a Release with the given build, name and creator. If any
+        of those is not given we use an arbitrary value.
+        """
+        if build is None:
+            build = self.make_build_result()
+        if name is None:
+            name = self.get_unique_string()
+        if creator is None:
+            creator = self.make_user()
+        return Release.objects.create(build=build, name=name, creator=creator)
+
+    def make_lexbuilder(self, name=None, uri=None, current_job=None):
+        """
+        Create a Lexbuilder with the given name, uri and current_job. If any
+        of those is not given we use an arbitrary value.
+        """
+        if name is None:
+            name = self.get_unique_string()
+        if uri is None:
+            uri = 'htt://example.com/%s' % name
+        if current_job is None:
+            current_job = self.make_build_result()
+        return Lexbuilder.objects.create(
+            name=name, uri=uri, current_job=current_job)
+
+    def make_build_request(self, project=None):
+        """
+        Create a BuildRequest, if no project is given, create a project and
+        associate the build with the newly created project.
+        """
+        if project is None:
+            project = self.make_project()
+        return BuildRequest.objects.create(project=project)
+
+
+factory = ObjectFactory()