[REVIEW] Progress bars for tree transforms

[John Arbash Meinel]

Aaron Bentley wrote:
...

> 
> Oh, boo.  What's the point of the special syntax if we can achieve the
> same effect with
> import bzrlib.ui
> ui_factory = bzrlib.ui.ui_factory ?
> 
> I think it would be better if ui_factory used the state pattern so that
> the object always stayed the same, but yes, I'll fix the code.

I think it is just to have shorter syntax. But yes, it doesn't do what
you want.

As far as ui_factory, I think the current code is a little bogus. I
would prefer bzrlib to maintain as little state as possible, to help
with thread safety. I'm okay with having factories, which can have more
objects registered to them. But just thinking about how one would write
a front-end which changes the ui_factory behavior seems a little clumsy,
but at least it is better than accessing ProgressBar directly.

> 
>>>>>> 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()):
> 
> So the fact that I'm supplying a default parameter that is a stateless
> do-nothing object is a concern?

Originally, I misread your code as:

def merge(...
          pb=ui_factory.progress_bar()):

which would be a concern, but you are right, using DummyProgress()
shouldn't be a problem. And I understand how it makes your code easier.

> 
>>>> No, I did this work on a separate branch.
> 
>>> 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).
> 
> All I want is repositories and I will be happy putting up endless
> feature branches.
> 
> But for right now, I'd rather send patches.  That patch should apply
> cleanly to bzr.dev, or anything closely related.
> 
> Aaron

I understand. The problem is that if I were to apply this patch, and you
merge it into one of your branches, it will conflict when I try to merge
you. (it happened before with test_find_missing.py).
Anyway, +1 from me. Feel free to merge it into bzr.dev yourself. You may
want to wait until Robert & Martin wake up, so they have a chance to
veto it in case I missed something.

John
=:->

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