Code review comment for lp:~spiv/bzr/per-file-merge-hook-491711

Vincent Ladeuil wrote:
[...]
> 41 +def cachedproperty(getter_func):
> 42 + """Like property(getter_func), but caches the value.
> 43 +
> 44 + Note: there's no way to clear or reset the cache.
> 45 + """
> 46 + memo = []
>
>
> AIUI, this is gc'ed when no more references exist on the cached
> value. It's not obvious and may be worth documenting.

Erk, this is totally bogus. I'm not sure what I was thinking! I was
trying for a limited quick & dirty cachedproperty decorator without the
complexity of the one in Launchpad, but I think I should just borrow
Launchpad's, so I've now done that (including arranging for its doctests
to be run).

> Also, this can be used in a lot of places in the codebase so
> that's worth a NEWS entry isn't it ?

Good idea. Done.

> The hook scope is a bit unclear regarding the tt updates (spotted
> when reading revno 4887). Why don't you let the hooks do them (in
> merge.merge_contents()) ? Shouldn't that be at least documented
> in the hook docstring ?

I'm not sure what you mean here? You mean let the hook do the
self.tt.version_file and self.tt.delete_contents calls that happen at
the end of merge_contents?

I guess I'm still not certain exactly how much the hook should be
allowed to do... the more plugins people try to write for it, the more
we'll know :)

[...]
> I'll be happy to land this for 2.1 unless John or Aaron have more objections
> until tomorrow.
>
> Addressing the points I raised above with be nice but not mandatory for landing.

That's very much for the extra review.

-Andrew.