[REVIEW] Progress bars for tree transforms
Aaron Bentley
aaron.bentley at utoronto.ca
Tue Feb 21 20:37:56 GMT 2006
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
John A Meinel wrote:
> Aaron Bentley wrote:
>
>>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:
Is this part of the confused bit?
> Anyway, because of this property, I always thought it was bad form to
> call a function as a default parameter.
The only thing close to that which I'm doing is using DummyProgress() as
a default parameter, which looks fine to me, because those objects are
cheap and stateless.
> I was being incomplete. My point was to do:
>
> def function(something, ..., pb=None):
>
> if not pb:
> pb = bzrlib.ui.ui_factory.progress_bar()
This appears to be a counterexample to
def function(something, ..., bzrlib.ui.ui_factory.progress_bar()):
But I'm not doing that anywhere.
>>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.
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 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?
>>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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD8DBQFD+3ok0F+nu1YWqI0RAn3mAJ0SShRV5+mRiU+szv/YK4168Ck/+QCggCxU
zkwIzLNrIw5zzVzJ9c3J61Y=
=aKVF
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list