Code review comment for lp:~jelmer/bzr/repo-size

Le samedi 26 décembre 2009 à 19:51 +0000, Robert Collins a écrit :
> On Sat, 2009-12-26 at 00:51 +0000, Jelmer Vernooij wrote:
> > > Generally we avoid giving objects that are not dicts or sequences a
> > > __len__, and I think its awfully prone to confusion here too, so we
> > > should not do that. I thought that this had been written down somewhere
> > > - you may wish to briefly look for it in the dev docs and if not found
> > > add a note to this effect: only make things look like containers if they
> > > feel like them too. (that is that you won't be surprised as a user).
> > >
> > > Rather, I suggest you put the revision count into the output of
> > > gather_stats: its not guaranteed to be cheap for all repositories, so
> > > there is every reason to make it optional.
> > As I mentioned in my merge request, it's already part of gather_stats()
> > but I'd like to have it in a separate function so I can override it for
> > subclasses. Would it be acceptable if I just renamed __len__ to
> > _get_revision_count() ?
> Or you could just delete printing the number of revisions. I don't think
> a method on repository is a particularly good idea; if anything a method
> on repository.revisions.
Except the foreign repository implementations don't necessarily
have .revisions set to something sane.

> However, gather_stats is *intended* to be
> overridden, trying to avoid overriding it is odd to me :).
>
> Another alternative would be to make the check for whether to attempt to
> calculate the revision count more sophisticated.
So, the reason I am trying to avoid overriding it is because it is quite
a big function and I would like to avoid duplicating all of that code in
all of the foreign branch plugins.

I think printing the number of revisions in "bzr info" is actually quite
useful, I've used it on several occasions.

Cheers,

Jelmer