Code review comment for lp:~mwhudson/launchpad/no-hosted-area-fix-useBzrBranch-users

On 26/04/10 20:58, Tim Penhey wrote:
> Review: Approve conditional
> I know that it is not worth it now, but in
> lib/lp/code/model/tests/test_branchjob.py you could
> have just overridden the inherited useBzrBranches method
> to make it always specify direct_database=True.

Yeah, this aspect of things does smell a bit doesn't it?

> It may be an interesting count, but which is used more?
> direct_database=True or False?

It seems 43 of 64 calls use direct_database=True -- quite a lot!

Part of my unease around this comes from the fact that what determines
whether to use direct_database or not basically depends on the layer --
if it's a appserver layer, don't use direct_database, otherwise do. I
don't know if tying it to the layer makes sense though.

What do you think?

> lib/lp/testing/__init__.py
> - you still have: raise AssertionError("XXX")
>
> removeSecurityProxy(branch).last_scanned_id = 'null:'
> - is there a bzrlib constant we can use here?

Yes, but that line disappears in a later pipe anyway I think...

> And yes, it was a pretty boring branch to review. Great
> to see a consistent use of getInternalBzrUrl and getBzrBranch.

Yeah!

Cheers,
mwh