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

On Tue, Sep 1, 2009 at 11:27 AM, Julian
Edwards<email address hidden> wrote:
> On Tuesday 01 September 2009 14:15:16 Celso Providelo wrote:
>> 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.
>
> Right, it should be clearer about the implications.
>
> But I feel it's the right thing to have in the majority of tests.  We want
> them to be as skinny and focused as possible.  Introducing packageupload
> creation in some cases causes the need for dbuser switching, which is really
> horrible to have in the middle of a doctest.

Right, I agree, but it only implies that the DB isolation aspect have
to be better encapsulated in the test helper, not necessarily that the
created objects should be incomplete.

After the patch, passing 'create_packageupload=False' means that the
returned object is not suitable for UI or disk publication test.

It is not only confusing for people trying to use the method (remember
we are FOSS) but also is controversial with the reasons why it was
introduced. If we want developers to use more 'naive' publications in
their tests because they are faster, why don't we make it easier to
use ?

One way to make it super-easy and clear to use is to port it directly
to the Factory:

* makeSourcePackageRelease(...)
* makeSourcePackagePublication(...)

I'm sure people will find it quicker than in STP and the confusion
will be gone. Use the factory normally and if you really need it
switch to STP (and think about improving the factory method)

(Sorry, it is getting way OT, we should start a separate thread for
this discussion)

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