launchpad-dev team mailing list archive

[Robert Collins <robert.collins@xxxxxxxxxxxxx>]

On Mon, Sep 13, 2010 at 1:38 AM, Gavin Panella
<gavin.panella@xxxxxxxxxxxxx> wrote:
> On 11 September 2010 22:04, Robert Collins <robert.collins@xxxxxxxxxxxxx> wrote:
> ...
>> This seems to draw a fairly large bow : adaption not working in one
>> context -> don't use adaption anywhere.
>
> Yeah, this is my main concern.

I'd suspect the tests failed because there are tests using a layer
without the zca present, that call code which previously didn't
actually /need/ any adapters etc. That would mean it ran without the
zcml parsed etc etc. Thats not consistent with 'only fails in a full
run' though. adding debug info is probably how I'd start tackling the
problem.

>> I think the declarative adaption facilities of the ZCA are great; but
>> that adaption isn't a panacea : the boilerplate you described would be
>> better written with delegation, not adaption.
>
> Fair.
>
> The example was taken from StructuralSubscriptionTargetMixin, and
> delegation would work well there. The classes that inherit this mixin,
> Product for example, are fairly big already so my thinking was along
> the lines of:
>
> - I wouldn't want to add yet more responsibility to these classes,
>
> - Being able to say "give me an object that lets me query and modify
>  structural subscriptions for this thing" sounds good,
>
> - Adaption seems like a good way to do that, and relieves the already
>  large classes from all responsibility,
>
> I might be wielding a hammer and seeing nails. Would you consider
> adapters for this scenario?

So, to paraphrase (checking I have it right):
 - there is common code to deal with things that can be structurally
subscribed to
 - there is some unique code per-type to deal with the same stuff

Without adaption I would probably have tackled it something like:
 - things that can be structurally subscribed to have a factory to
give you their structural subscription object
 - that object is tailored to any uniqueness needed
 - I would not have used a MixIn

With adaption, I think I would:
 - have an adapter to get IStructuralSubscriptionTarget from things
that can be subscribed to
 - the resulting IStructuralSubscriptionTarget is tailored to any
uniqueness needed

So, if the current situation is that ISST adaption is done by using a
mixin on e.g. Product, then I think that you should either:
 - customise the mixin per target type (mixin
StructuralSubscriptionTargetProduct to Product,
StructuralSubscriptionTargetDistribution to Distribution) - the
uniqueness will be in the mixin
 - stop using a mixin and actually use an adapter :)

What I definitely wouldn't do is half and half, it seems likely to
neither add clarity, nor clear things up.

My personal bias is something like:
 - use dependency injection techniques when doing dependency injection
 - use simpler code otherwise

But thats handwavy and not entirely true ;P.

>> # Note that writing it this way makes it reasonably clear that we
>> # aren't using delegation/composition as much as we could in this area.
>> self.populateArgs()
>>
>> Similarly, I'm very unconvinced that using adaption for the property
>> cache was useful or helpful. I like the new API, but it seems like it
>> could be more pithy, more reusable and faster if it was just plain
>> python.
>
> I was tempted by the ZCA! I honestly thought we might need the
> flexibility that could bring, but turns out we didn't.

So for the property cache the question is : will we want a different
implementation of the cache while testing/deploying.

I think the answer is 'No', so while adaption is prettty quick, its
still almost certainly slower than just deferencing the object to get
the cache.

> So the best way to fix bug 628762 is probably to do away with
> adaption, but I'd like to repair my confidence in Zope too (or repair
> my knowledge).

I'm happy either way: you've stepped up and made the cached property
API much cleaner; whether it uses ZCA is a relatively minor detail.

If there is a zerious issue in the ZCA, knowing and fixing it is important!

-Rob