[MERGE] Change CommitBuilder factory delegation to allow simple declaration.

Robert Collins robertc at robertcollins.net
Sun Sep 16 01:23:39 BST 2007


On Fri, 2007-09-14 at 22:08 -0400, Aaron Bentley wrote:

I'll change the code to be self._commit_builder_class because I want the
thing landed, but I still don't agree with your assessment of the use of
__class__ - see below for more discussion. (But to be clear; I'm making
your requested change anyway).

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Robert Collins wrote:
> > On Fri, 2007-09-14 at 16:46 -0400, Aaron Bentley wrote:
> >>> +        result = self.__class__._commit_builder_class(self, parents, config,
> >>> +            timestamp, timezone, committer, revprops, revision_id)
> >> I don't see why you're preventing overloading _commit_builder_class on
> >> particular instances of Repository.  I think self._commit_builder_class
> >> is more suitable.
> >>
> > 
> > Our test suite only understands variation by format at the moment and
> > every format has a dedicated class, so varying by instance does not
> > assist us and could encourage the introduction of untested commit
> > builders; I don't want to encourage that until/unless someone steps up
> > with a use for it - and the changes (whatever they may be) to get it
> > tested.
> 
> In Python, instance variables can override class variables, and as far
> as I can tell, this is by design.  If this is a real concern of yours,
> you haven't gone nearly far enough to prevent it.  For starters, commit
> would need to do
> 
> self.builder = self.branch.__class__.get_commit_builder(self.branch ...
> 
> instead of
> 
> self.builder = self.branch.get_commit_builder(
> 
> Because methods are variables, too.  Your code doesn't look safer to me
> in any meaningful way.  It looks like an arbitrary, ugly, violation of a
> language feature.

While pythons anything to be overridden it is unconventional to alter
objects behaviour by assigning to their methods; theres even a specific
name for when this is done - 'Monkey patching'. 

Its very hard in python to prevent things (without C extensions anyhow);
what I was doing was not _preventing_ deliberate abuse; it was
preventing casual use of 'self._commit_builder_class = BLAH'  - which is
something that there is no visual indicator that it would be unusal to
do it - from causing untested code to be used (as explained before).

Overriding methods is much more commonly done by subclassing, which will
hook into the test suite - and this combined with the fact that python
depends on cooperation not prevent is why I felt what I had done was
both useful and sufficient.

Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070916/55704dc0/attachment.pgp 


More information about the bazaar mailing list