Code review comment for lp:~jml/launchpad/package-permission-love

On Monday 31 August 2009 03:51:07 Jonathan Lange wrote:
> > Whenever I see exceptions used, I ask myself "is this caused by an
> > exceptional circumstance?" In this case, I'm not sure it is, but I can
> > see why you've used one - so that you can return an error string as well.
> > Is that a good enough reason to use an exception here? I guess the
> > code's neater but I still have a nagging feeling that it's bad form.
> >
> > How does this look in comparison:
> >
> > error = verify_upload(
> > signer, source_name, archive, self.changes.dsc.component,
> > not self.is_new)
> > if error is not None:
> > self.reject(error)
>
> I've never really understood the reasoning behind the "exceptional
> circumstance" argument.

Exceptions should be thrown only when there is an abnormal condition that the
code cannot handle. Exceptions should convey: "Woa! Something went wrong
here and I can't deal with it. BOOM." In this case, you're using them as a
routine return value, which doesn't fit that requirement. Also bearing in
mind that they're pretty expensive, if a piece of code can return a value to
indicate its results, then it should do so.

Obviously, "abnormal condition" is a little subjective, but I think in this
case it's clear cut, at least to me, since you're asking for a yes or no
answer.

> I've changed the code to return reason objects rather than raising
> exceptions -- returning a string is too ugly for me right now. This
> change doesn't effect the structure of the function at all.

Okay, it looks better, thanks.

> I've also added an assertCannotUpload to the test case and made the
> tests that check for negative conditions use that.

Excellent, thanks.

> > I suspect a lot of our existing tests for ACLs are now duplicated by your
> > new ones. I don't think we need to fix that in this branch, but it would
> > be great if you could file a bug about that so we remember to check/fix
> > in the future! I love deleting un-needed tests.
>
> Filed bug 421702 to track this.

Thanks.

> I also love deleting unneeded tests.

And correcting my grammar ;)

> mergeFunctionMetadata(f, g) takes the __name__, __doc__ and other
> properties from 'f' and mutates 'g' so that it has the same name,
> docstring and other properties. This is particularly useful for
> decorators, since the returned function (the modified 'g') has a
> useful name when it appears in a stack trace, rather than 'decorated'
> or 'function' or something equally miserable.

Ah! Nice.

> I did a merge from stable while replying to reviews, so I can't
> generate an interdiff easily.

:( I hope the Code diff updating stuff lands soon. Although I don't think it
generates partial diffs. Ho hum.

> Do I still need to wait for cprov's review?

Well he told me he'd done a review, but had not replied yet. I'll ask him
when he's online and reply to confirm.

Cheers
J