Code review comment for lp:~vila/bzr/323111-backup-names

>>>>> John Arbash Meinel <email address hidden> writes:

    > On 9/10/2010 9:40 AM, Vincent Ladeuil wrote:
    >> + def test_resolve_conflicts_missing_parent(self):

    > The available_backup_name seems ok. I'm not a big fan of 'transport.has'
    > or the look-before-you-leap in general, but we were already doing it.

    > I don't really understand what this is doing here, though:
    > + def test_resolve_conflicts_missing_parent(self):

    > It seems unrelated. Also why remove:
    > - def test_resolve_no_parent(self):

I've replaced it by test_resolve_conflicts_missing_parent clarifying it
while I was there.

    > === modified file 'bzrlib/workingtree.py'
    > --- bzrlib/workingtree.py 2010-08-20 19:07:17 +0000
    > +++ bzrlib/workingtree.py 2010-09-10 14:39:52 +0000
    > @@ -2078,9 +2078,10 @@
    > files_to_backup.append(path[1])

    > def backup(file_to_backup):
    > - backup_name =
    > self.bzrdir.generate_backup_name(file_to_backup)
    > + backup_name =
    > self.bzrdir._available_backup_name(file_to_backup)

    > ^- IMO: WorkingTree shouldn't be calling an '_' method on BzrDir,

Wow, we do that a lot already, what's the problem ?

The public methods or attributes are for bzrlib users while the private
ones are for bzrlib internal needs, we never say this applies to inter
class calls.

    > so we should probably be making it public.

IMHO, methods should be public if there is a need for them outside of
bzrlib and we guarantee some stability and backward compatibility.

If there is no need, the maintenance cost is lowered.

    > Then again, why is *BzrDir* finding the available name, and not
    > working tree itself?

Well, I just touched this method lightly, I didn't research the
rationale in the corresponding merge proposal.