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

On Tue, Sep 1, 2009 at 8:24 AM, Julian
Edwards<email address hidden> wrote:
> 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 don't feel totally against the previous approach of raising an
exception representing the reason why the upload was rejected,
although I'd expect it to return a boolean by its name.

If one day we decide to store rejected uploads (for tracing/stats
purpose) we will probably have to map the rejection reasons to a
DBEnum, which would still make sense since there is indeed only a
limited number of situations covered by this code.

The truth is, the proposed change already results in much clearer
callsites, the differences between raising or returning the
rejection_reason doesn't affect it. So, thanks for it :)

[snip]

>> 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.

I'm sorry for blocking this change.

The only comment I have is about the new control variable added in
STP.getPubSource(). Why would you ever need to create an source
publication that was *never* uploaded and doesn't contain any file ?
It will look like a source imported by 'gina' but with no files.

I suppose you are trying to avoid hitting the librarian and it will
certainly speed you test up as long as the logic doesn't need that
logic, but I don't think those implication are clear in the method
docstring as they should.

Since there isn't any callsite using it, I guess that change doesn't
need be landed right now.

Source creation is really simple and doesn't really depend on any
distroseries (chroot) setup, it won't be that difficult to move it to
the Factory and once chroot-procedures get integrated in the DAS api
it won't be any harder to move binary creation methods either.

Let me know what you think.

--
Celso Providelo <email address hidden>
IRC: cprov, Jabber: <email address hidden>, Skype: cprovidelo
1024D/681B6469 C858 2652 1A6E F6A6 037B B3F7 9FF2 583E 681B 6469