Code review comment for lp:~james-w/launchpad/archive-collection

On Mon, 02 Aug 2010 15:58:40 -0000, Jonathan Lange <email address hidden> wrote:
> What's the reason for this change? Where are the corresponding tests?

Sorry, I forgot to mention it in the cover letter.

When you register a Collection as a utility it is instantiated by the
zcml.

However, as the store is queried in the constructor this can fail if
there is not a utility registered for the store yet.

How this manifested was that as soon as I added the utility I could no
longer run any tests as there was an exception during zcml processing.

I'm not sure how I would test this directly? A test that created a
Collection and tested that .store was None?

It does make me realise that I should probably add a test that the
utility returns something sensible?

> In IBranchCollection, we have a *args method for something similar. Do
> you think that would be nicer here?

The implementation is *purposes, so the interface needs correcting.

> Maybe it's my crappy email client, but the indentation on this looks messed up.

It's non-standard as it was copy-paste code, I will fix.

> > +        # FIXME: Include private PPA's if user is an uploader
> > +        return self.refine(
> > +            Or(public_archive, Archive.ownerID.is_in(user_teams_subselect)))
> >
>
> Are you going to fix this in this branch? Won't this be buggy if
> someone uses it?

This is copy-paste code, so we apparently have survived this far without
needing it.

I can look at fixing it though.

> These tests are good, thanks.
>
> One thing I've done with similar tests is have a setUp() that removes
> all of the data in question (e.g. removing all branches). That way you
> can do actual equality testing rather than testing to if if objects
> are members in tests.
>
> You don't have to though.

That's a good idea, I'll look for your code to learn from it.

Thanks,

James