[RFC] Splitting bzr-specific functions out of Repository

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Collins wrote:
| On Tue, 2008-07-29 at 03:01 +0200, Jelmer Vernooij wrote:
|> Am Montag, den 28.07.2008, 09:48 +1000 schrieb Robert Collins:
|>> On Sun, 2008-07-27 at 21:05 +0200, Jelmer Vernooij wrote:
|>>> Would anybody be opposed to splitting the bzr-specific functions out of
|>>> Repository into a BzrRepository class that is a subclass of Repository,
|>>> similar to BzrBranch and Branch?
|>> I find the bzrbranch/branch split to be quite unsatisfactory. It has
|>> lead to some [transient] bugs in fact.
|> We don't have any other implementations of Branch in bzrlib that are
|> exercised by the tests other than the implementations that derive from
|> BzrBranch. That probably hasn't helped; it may be useful to add some
|> tests specifically for Branch.
|
| If we have abstract functions that noone can/should override - sure; but
| generally if its ok for someone to override a method we should test the
| overridden methods.
|
|>> Presumably you're suggesting this to make your life easier :) - what in
|>> particular is causing you friction/headaches? (Just having the methods
|>> there is at most unclear). Is it needing a more clear definition of what
|>> you need to implement, or ???
|> Having the methods there is unclear, also means there's no easy way for
|> me to determine what I do and what I don't have to implement: other
|> parts of Bazaar seem to rely on _ functions even though they are
|> underscore functions.
|>
|> This is probably boils down to the discussion we had in London about
|> various degrees of accessibility different functions have in Bazaar.
|
| Right. For the list: we were talking about how things (like the CLI)
| inside bzr use methods that we do not export to arbitrary users of
| bzrlib. Sometimes this is because we are not ready to mark something as
| a public method.
|
|
|>> If we don't want these on the public interface, I'd rather we do the
|>> deprecation dance and give them _ names.
|> Giving them _ names won't guarantee that other parts of bzrlib won't
|>  try to run them.
|
| Neither will taking them off the base class - python isn't c++ :).
|
|> bzr-svn was implemented by trying to run various bzr commands, seeing
|> what Repository methods were being executed and then implementing those
|> methods. It has happened in the past that _ methods that weren't used by
|> any of the builtin commands started being used in some code path
|> executed by the builtin commands, breaking bzr-svn. It's also possible
|> that I didn't properly exercise all code paths that may use _ functions
|> while testing bzr-svn.
|
| I would *dearly* love to have the bzr test suite be capable of running
| the full set of bzr-svn tests - interface and branch conformance. This
| is really the key to prevent regressions in support IMO.
|
|> It would be much nicer (and work on bzr-git is what brought this up
|> again for me) if it was possible to see easily what methods I would have
|> to implement.
|
| When I implement a radically different repository format, I lean on the
| bzr interface tests very heavily. They are the only exhaustive list that
| I know of for checking behaviour.
|
|> Another way of working around this (instead of adding another level of
|> subclasses) would be of course be to have the _ and __ functions, _
|> meaning "not part of the public API but part of the interface" and __
|> meaning completely private.
|
| The problem is that __foo can't be overridden. So this makes
| _overridemeinsubclasses be part of the interface always, even when it
| may not be.
|
| -Rob

I've started taking the "_do_XXX" to mean override-me-in-subclass, which
seems to work well. It also gives a point when you are being clear that
you expect this class to be subclassed and implemented. It also gives a
double-abstraction. So that if you want to change the public "foo" you
can still thunk over to the old _do_foo when the *new* _doo_foo_better
is not available.

It is a bit of a pain to implement all of your classes as double
abstract (abstract to callers, abstract to implementations). But we
probably would see a compatibility benefit for the WT/Branch/Repo
classes, just because those are fairly heavily re-implemented.

At least we can try to keep that in mind as we go forward.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkiOf1kACgkQJdeBCYSNAAPGxgCfa2a8kzhj3GKfH9yVGThI1gZI
Tr8AoMPWWOAqd3cbnLgx+uZAtHlbgYOz
=ppB0
-----END PGP SIGNATURE-----