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

On Fri, 2009-12-25 at 15:36 +0000, Jelmer Vernooij wrote:
> Jelmer Vernooij has proposed merging lp:~jelmer/bzr/repo-size into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #123653 "bzr info" depends on private Repository._revision_store
> https://bugs.launchpad.net/bugs/123653
>
>
> This patch adds a custom implementation of Repository.__len__ that
> returns the number of revisions in the repository.
>
> This is mainly convenient for foreign branch plugins, which do not
> provide Repository.revisions. Currently Repository.gather_stats() relies
> on len(Repository.revisions), so foreign branch plugins have to override
> it.

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.

 review needs-fixing