[MERGE][Trivial] invalid version tuple attempt to raise AssertionError fails with TypeError

Wed Sep 24 04:56:44 BST 2008

I'm sorry, I didn't take your suggestion seriously because I was
distracted by the bit about bzrtools.

> On the other hand, a simple grep shows bzr-svn using:
> 
>  from bzrlib.plugins.svn.transport import _url_escape_uri
>  from bzrlib.plugins.svn.commit import _revision_id_to_svk_feature

This shows bzr-svn importing private symbols from itself.  I don't think
there's a problem with that, and there's certainly no way that stock
bzrlib can address it.

>  from bzrlib.diff import _patch_header_date  (in a test)

This is a candidate, yes.

> QBzr:
> 
>  from bzrlib.annotate import _annotate_file

Although QBzr imports this symbol, it doesn't seem to actually use it.

>  from bzrlib.diff import _get_trees_to_diff

I'm inclined to agree with you here.

>  bzrlib.builtins.cmd_merge._do_preview(self, merger)

I'm quite hesitant to treat command implementations as part of the API.

>>> and these plugins *do* use other private symbols,
>> In violation of our policy.
> 
> Shock Horror!  How Dare They!  It might be worth keeping in mind that we are
> talking code owned by others here, so your policy isn't relevant.

We are in a community.  Treating one another with respect helps that
community grow.  If the authors of Qbzr thinks that _get_trees_to_diff
is a useful function, it would be really nice if they told us so.
Making it public is good for other bzrlib users, and it's also good for
them, because then they don't have to worry that one day it will
suddenly break on them.

>> Old code tends to be worse code.
> 
> Heh.  Surely *some* of that old code is good, solid reliable code?

Sure, some of it is.

But let's consider some of the code you cited: the QBzr code that
imported _annotate_file.  When the code was new, there was a reason for
that import, but not anymore.  (I know, 'cause I used gannotate :-)

For plugins in particular, there's always the possibility that their old
code uses an old private API, when a new public API would work as well
or better.

> I
> understand its very easy to think all old code must be bad/dead/whatever,
> but that often simply means the code isn't understood completely.

Certainly not all old code is bad.  But our newish code is shows a
better understanding of the problem domain, more consistent style, tends
to perform better, and has less cruft.

>>> I think I've done my duty :)
>> I think that it would have been easy to make the symbol public at the
>> same time as fixing the bug.  I didn't say you had a duty, I made a
>> request.
> 
> You made a request that I "Please help us make our API great." and you made
> this request after I found the problem, submitted the patch, responded to
> your suggestion the API be changed and grepped the source for 4 bzr
> extensions for other more suitable private functions being used publically.

I hope that those weren't carefully considered suggestions, because 4 of
7 were completely unsuitable:
- - bzrlib.revision
- - bzrlib.plugins.svn.transport._url_escape_uri
- - bzrlib.plugins.svn.commit._revision_id_to_svk_feature
- - bzrlib.annotate._annotate_file

> So from my POV, I had *already* tried to be significant help *before* you
> asked me to offer that help.

I had a different impression of your motivations.  I though you were
trying to bolster an argument that using private symbols was okay
because everyone's doing it.  Sorry about that.

> It almost comes across as though my previous
> efforts on this bug were actually a hindrance, or somehow not *enough* help.

Well, to an extent, every review costs time, so there's a motivation to
make the most of it.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFI2bp80F+nu1YWqI0RAgidAJ95BwEVKRNTQNQtswzo5Q3jkxPtzACfdPvw
CBBHe6gVB3Lnhgf0QkRaNeA=
=Q6g4
-----END PGP SIGNATURE-----