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

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

-Rob