[REVIEW] Progress bars for tree transforms

[John A Meinel]

Aaron Bentley wrote:
> John A Meinel wrote:
>>> @@ -1749,7 +1751,8 @@
>>>          try:
>>>              conflict_count = merge(other, base, check_clean=(not force),
>>>                                     merge_type=merge_type,
>>> reprocess=reprocess,
>>> -                                   show_base=show_base)
>>> +                                   show_base=show_base,
>>> +                                   pb=ui_factory.progress_bar())
>>>
>>>
>>>
>>> Doesn't this create a default progress bar at first import time, rather
>>> than creating a new one each time the function is called?
> 
> ui_factory.progress_bar is called when you execute cmd_merge.run()

Really. Hmm... I just know that with python the only way to create a
'static' variable is to do:

>>> def function(x, y=[]):
...   y.append(x)
...   print y
...
>>> function(1)
[1]
>>> function(1)
[1, 1]
>>> function(1)
[1, 1, 1]
>>> function(1)
[1, 1, 1, 1]
>>> function(1)
[1, 1, 1, 1, 1]
>>> function(2)
[1, 1, 1, 1, 1, 2]

Anyway, because of this property, I always thought it was bad form to
call a function as a default parameter.

> 
>>> I think if you are doing this, then:
>>> pb = None
>>> if not pb:
>>>   pb = ui_factory.progress_bar()
> 
> The conditional will always evaluate to True, so what's the point?

I was being incomplete. My point was to do:

def function(something, ..., pb=None):

  if not pb:
    pb = bzrlib.ui.ui_factory.progress_bar()


> 
>>> But remember, we shouldn't do 'from bzrlib.ui import ui_factory' since
>>> that creates a local variable, and the way you install a new factory is
>>> by overriding that variable.
> 
> AIUI, imports do not create new variables, they make existing names
> visible in another namespace.  The first import would be an exception,
> obviously, because the module or package is initialized then.
> 

The problem is that if you do "from bzrlib.ui import ui_factory",
and then later on I do:

import bzrlib.ui
bzrlib.ui.ui_factory = AnotherFactory()

At that point your code will still be using the original factory.

Robert fixed this in another portion of the code.

doing "from bzrlib.ui import foo" is binding the name 'foo' in the local
namespace to the current object int bzrlib.ui.
If someone later on changes bzrlib.ui.foo you do not see that change.
Which is why you need to do something like:
import bzrlib.ui as ui

So that the local name points to an object which won't be changing (the
ui module).

The reason your code seems to be working, is because you aren't
importing the 'transform' until after main() has been called, so it has
had a chance to override the default SilentFactory.

But if we changed the order of imports, all of a sudden the transform
code would stop displaying a progress bar, because it would have grabbed
the default ui_factory.

>>> I also prefer passing in the progress bar, so that the calling code can
>>> decide if a progress bar should be displayed, rather than the code
>>> always creating its own progress bar.
> 
> The only caller is run_bzr_catch_errors, and it never specifies whether
> progress should be permitted.

At present, that is the only caller.

I just realized that I was cutting out the wrong portion of the code,
sorry for the confusion.

My concern was more for things like this:
 def merge(other_revision, base_revision,
           check_clean=True, ignore_zero=False,
           this_dir=None, backup_files=False, merge_type=Merge3Merger,
-          file_list=None, show_base=False, reprocess=False):
+          file_list=None, show_base=False, reprocess=False,
+          pb=DummyProgress()):


> 
>>> Is this in your 'bzr.ttransform' branch? I would like to pull it so I
>>> can see the actual progress bar work. Which is something I am looking
>>> forward to.
> 
> No, I did this work on a separate branch.
> 
> Aaron

Okay. Do you have this accessible? I personally prefer to work with
merges rather than patches. (Maybe I need to re-prioritize my changeset
work).

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/20060221/d2e9367e/attachment.pgp