[PATCH V2] New factory class to create arbitrary model objects, to be used in tests

[Guilherme Salgado]

Hi Jeremy,

On Fri, 2011-04-29 at 09:51 +0800, Jeremy Kerr wrote:
> Hi Guilherme,
> 
> > > > +# Patchwork - automated patch tracking system
> > > > +# Copyright (C) 2011 Jeremy Kerr <jk at ozlabs.org>
> > > 
> > > Probably best to add your name here :)
> > 
> > Should I add myself as the author and keep you as the copyright holder?
> 
> Since I haven't written anything in that file, best just to put your
> name there.

Ok, I've done that.

> 
> > > 
> > > > +
> > > > +    def __init__(self):
> > > > +        # Initialise the unique identifier.
> > > > +        self.integer = count(randint(0, 1000000))
> > > 
> > > As a general rule, I'm averse to using random data in test cases. We may
> > > see non-repeatable failures here, if we create multiple ObjectFactories
> > > that happen to have colliding unique IDs.
> > > 
> > > So, perhaps we should make ObjectFactory a singleton (referenced though
> > > an exported module variable, something like 'factory'), and just use:
> > > 
> > >     self.integer = count()
> > > 
> > > How does that sound?
> > 
> > I can't think of any reason for not having ObjectFactory as a singleton,
> > and I think your concern is valid, so consider this done. :)
> 
> OK, great.
> 
> > Indeed. I've added 'content' as an optional argument and when it's not
> > passed I set it to a minimal patch.
> > 
> > I'll submit a third version with these changes as soon as I have an
> > answer from you about the copyright/authorship question. Oh, and I'll
> > also submit new versions of the patches that use this as they now use
> > the factory singleton.
> 
> Sounds good, thanks. When you re-roll this patch, could you avoid
> breaking lines right after the opening bracket? So instead of:
> 
>         person = Person(
>             email=self.getUniqueEmailAddress(), name=self.getUniqueString())
> 
> We get:
> 
>         person = Person(email=self.getUniqueEmailAddress(),
>                         name=self.getUniqueString())
> 
> Also, most of the codebase has spaces around the '=' in kwargs:
> 
>         person = Person(email = self.getUniqueEmailAddress(),
>                         name = self.getUniqueString())
> 
> - I'm aware that this is not what PEP-8 says, but I'd prefer to keep it
> consistent, and perhaps do a global change at some point.

Ok, I've changed this and will keep it in mind for future patches.

> 
> Also, the initial state is defined as one with ordering = 0:
> 
> +        patch = Patch(
> +            project=project, msgid=msgid, name=self.getUniqueString(),
> +            submitter=submitter, date=date,
> +            state=State.objects.get(name='New'))
> 
> This should be:
> 
> 	state = State.objects.get(ordering = 0)
> 
> - as there may not be a 'New' state in a particular patchwork setup.
> Even better, you can leave 'state' unset, and the Patch.save() will set
> it to this default.

Cool, I've removed that, then.  Will send an updated patch shortly.

Cheers,

-- 
Guilherme Salgado <https://launchpad.net/~salgado>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/patchwork/attachments/20110429/32d3c5b7/attachment.pgp>