[RFC][MERGE] bzrdir phase 3

[John A Meinel]

Robert Collins wrote:
> On Thu, 2006-02-16 at 20:28 -0600, John A Meinel wrote:
> 
>>>> Doesn't this need to be declared 'staticmethod'?
>>> No. The comment there should explain it. Its a quirk of the way the
>>> command classes are used; I haven't spent the time digging into it yet
>>> to explain it.
>>>
>> Looking at it, it is just being passed to Option() as the way to
>> interpret get_format_type. I understand what it is doing. But I would
>> recommend that we move it outside of the class, just like all of our
>> other parsers. Plus it would make it easier to write tests for it in the
>> future.
> 
> I'll move it out of the class then. I'm not sure where to put it though
> - it seems awfully kludgy for options.py to centralise everything. For
> now I'll put it above the class in builtins.py. Note that it doesn't
> actually make testing any easier or harder ;0.

I don't have a problem with it being there. Or being inside the class
and declared a static method. (We do have centralization for errors.py,
but that is something else).

I just did some testing, and it without staticmethod you cannot call
this function directly. Specifically, I did this:

>>> class a(object):
...   def func(foo, bar):
...     print foo, bar
...
...   m = func('a', 'b')
...
a b
>>> x = a()
>>> x.func('c')
<__main__.a object at 0xb7ecf90c> c
>>> a.func('foo')
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
TypeError: unbound method func() must be called with a instance as first
argument (got str instance instead)
>>> a.func('foo', 'bar')
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
TypeError: unbound method func() must be called with a instance as first
argument (got str instance instead)

Without staticmethod, it works inside the scope of creating the class,
but outside of that class, you must pass an instance as the first parameter.

Thus you can't test the function. (Other than blackbox testing, to make
sure 'run' gets the right argument.). But you can't write a test which says:

self.assertRaises(BzrCommandError, cmd_diff.get_format_type, 'not-metadir')


> 
>>>>> === modified file 'bzrlib/bzrdir.py'
>>>>> --- bzrlib/bzrdir.py	
>>>>> +++ bzrlib/bzrdir.py	
>>>>> @@ -21,18 +21,32 @@
>>>>>  """
>>>>>  
>>>>>  from copy import deepcopy
>>>>> +import os
>>>>>  from cStringIO import StringIO
>>>>>  from unittest import TestSuite
>>>>> -
>>>>>  
>>>>>  import bzrlib
>>>>>  import bzrlib.errors as errors
>>>>>  from bzrlib.lockable_files import LockableFiles
>>>>>  from bzrlib.osutils import safe_unicode
>>>>> +from bzrlib.osutils import (
>>>>> +                            abspath,
>>>>> +                            pathjoin,
>>>>> +                            safe_unicode,
>>>>> +                            sha_strings,
>>>>> +                            sha_string,
>>>>> +                            )
>>>> Why do you have the closing ')' line up with the entries, rather than
>>>> lining up with the opening '('. This isn't the only place where occurs
>>>> (as I recall the test name listing does this as well). I think it would
>>>> look better with the () lined up.
>>> In Launchpad the coding style is that such things line up with the
>>> content like I do here, I don't recall if thats PEP8 or just a visual
>>> aid. Regardless of that we can choose our destiny... but I like it
>>> lining up with the indented entries, its less distruptive to the eye
>>> while still being trivial to add lines to.
>>>
>> You don't think:
>> from bzrlib.osutils import (
>>                             abspath,
>>                             pathjoin,
>>                             safe_unicode,
>>                             sha_strings,
>>                             sha_string,
>>                            )
>>
>> Looks better? I guess I'm used to C++ braces lining up to make it clear
>> what the section is. I always use either:
>>
>> if (something) {
>> }
>>
>> or
>>
>> if (something really long that it doesn't fit on a line)
>> {
>> }
>>
>> I think it would look funny to do
>>
>> if (something)
>>   {
>> }
> 
> Thats not equivalent though, and I agree that that would be funny.
> if (something) {
>                 something++;
>                 }
> 
> Is the equivalent layout, and I think it makes the block very visible to
> the eye.
> 

I still prefer the look of:

if (something) {
                 something++;
               }

But it is an aesthetic, and thus bound to differ between people.

> 
>> But maybe that is just me.
>>
>> ...
>>
>>>> The problem is 'BzrFormat6.needs_format_upgrade(BzrFormat4) would return
>>>> True, even though it is 'downgrading' the format.
>>> Thats correct though: if someone wants to change the format to be
>>> BzrFormat4, the will need to run a format converter.
>> I guess calling it 'upgrade' is confusing. Calling it something like
>> "needs_conversion" would be more accurate.
> 
> ok, s/update/conversion/ being done.
> 

Good.


> Ok. Well we can use the metadir format as the time its 'active' then.
> And avoid an explicit bump for it.

That is what I was thinking.

> 
>>> that would output a meta 1 bzr dir, format 7 repo, format 3 working tree
>>> and BoundBranchFormat format branch. 
>>>
>>> But importantly: the source format is provided by your plugin, the
>>> target bzrdir format is -not-.
>>>
>> It just depends on how you look at it. You provide *a* format. It could
>> be a source or a target. Depending on which way you are upgrading.
> 
> Sure. I was highlighting that in response to your comment about the
> target format owning the converter.
> 
>>> And finally, the reason the api is on the source format is that that is
>>> the order we walk the conceptual graph: SOURCE->OUTPUT, rinse and repeat
>>> until we are done.
>>>
>> I kind of see that, and its certainly how it has been done. But I could
>> also see finding the path from source to output, (say with graph
>> search), and then apply the proper converters.
>>
>> When I provide a plugin with a new format, it would make sense that I
>> would know how to convert from the main format to my custom format. But
>> it would be very wrong of me to assume that the main format would know
>> how to convert my custom format.
> 
> But.. its also wrong to assume that as the mainline evolves it will know
> how to convert *from* your format.
> 

True, hence why you need to provide both converters, and it shouldn't be
owned by either one.

>> Hence why I think it shouldn't be part of either end, and should just be
>> a general registry of arcs in a graph.
> 
> The driver data will be. The api will still use a call on the source,
> because a simple equality test is not sufficient, and being sufficient
> would require breaking abstraction.
> 
>> ...
>>
>>> We can start using the Meta-1 format when we decide its time to start
>>> guarantee support for branches in it - we just freeze the format and
>>> bump to a new one internally. I think letting the lock code get in is
>>> good, but I'd like an intermediary format between the weave and the knit
>>> transition. So, we could start using this now I think.
>>>
>> I would also like to see at least 1 format bump before knits. I would
>> like to get a couple more things into Meta-1 format. I actually have
>> written escaped stores in a moderately robust manner so someone can use
>> a baz->bzr branch on windows, and I would like to see bound branches in
>> Meta-1.
>> Otherwise we would again have some clients that support these things,
>> and some clients that don't. All which *think* they support the same format.
>>
>> By the way, for escaped stores, we could make it a property of the
>> format (All metadir format 1 branches are escaped), or we could make it
>> an orthogonal property (format 1 branches with escaped=True have escaped
>> stores).
> 
> I am in favour of 'all metadir format 1 branches are escaped'. I'll
> happily +1 an addition to the format6-meta1 converter to escape the
> stores.
> 

I'll look into that....

> 
> 
>> Certainly not for this patch. I was just curious if there was a specific
>> plan for using gettext.
> 
> Not yet AFAIK. I think we should just bite the bullet and put it in, and
> let translations come in incrementally.
> 

That is kind of what I thought. It is a large number of small changes,
affecting almost the entire codebase, and will conflict like hell.
Better to do it sooner rather than later. And we could also make our
wrapper turn all strings into unicode objects if we wanted. (Maybe
gettext does that anyway)

>> ....
>>
>>>>> +        self.assertEqual("\r                                                                               \r", stderr.getvalue())
>>>>>
>>>> This looks like a syntax error. Which means the code is not being run.
>>> nah, its your terminal or our diff getting confused.. Thats the literal
>>> output from a progress bar.
>> I figured out the problem. It is just a really long line, so I couldn't
>> see the end of it. And I didn't think to scroll.
>>
>> I think you violated 80 characters yourself. :) But it would be much
>> easier to read as '\r' + (' '*75) + '\r'. And easier to maintain. Rather
>> than trying to count the whitespace.
> 
> Will do.
> 
> Do I have +1 with the above done ?
> 
> Rob
> 

Sure. +1 from me.

John
=:->



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060217/b68e05db/attachment.pgp