Code review comment for lp:~salgado/launchpad/upload-policy-utility

 merge approve
 review approve

On Wednesday 18 August 2010 14:25:58 you wrote:
> On Wed, 2010-08-18 at 13:13 +0000, Julian Edwards wrote:
> > Review: Needs Information
> > Well I guess I meant both :)
> >
> > So considering:
> >
> > - policy = findPolicyByName(name)
> > + policy = getUtility(IArchiveUploadPolicy, name)()
> >
> > I find the former far more readable - it actually tells me what it's
> > doing *and* more importantly it's abstracting out *how* it works so we
> > can tweak it more easily later if we want. The fact that it's the
> > same line count doesn't really matter - in fact your diff would have
> > been much smaller :)
>
> I see; that sounds ok to me. I'll do that change.

Great!

>
> > As for moving out setOptions(), I think that's OK, but given my
> > reasoning above, would you agree it makes sense at least to keep the
> > old names?
>
> names? Do you mean to keep findPolicyByOptions() as well? I don't see
> any reason for doing that as it just uses options.context to lookup a
> policy by name -- I'd rather have the callsites use
> findPolicyByName(options.context) instead. How does that sound to you?

Ah yes of course, that's good.

Land it! Thanks for the improvement to Soyuz.