Code review comment for lp:~mbp/bzr/387717-progress-bar-tty

2009/6/20 John A Meinel <email address hidden>:
> @@ -82,7 +91,9 @@
>         self.total_cnt = None
>         self.current_cnt = None
>         self.msg = ''
> +        # TODO: deprecate passing ui_factory
>         self.ui_factory = ui_factory
> +        self.progress_view = progress_view
>         self.show_pct = False
>         self.show_spinner = True
>         self.show_eta = False,
>
>
> ^- Why not just deprecate it?
>
> if ui_factory is not None:
>  symbol_versioning.warn(
>    'Passing ui_factory' % symbol_versioning.deprecated_in((1, 17, 0)),
>    ' Pass progress_view instead')
>
> or are there still bits that use the ui_factory without going to
> progress_view?

I though there might be some nontrivial work to deprecate it and to
look into how that would impact bzr-gtk, so I wanted to land this
first. (And from Vincent's comments it looks like that's true.)

That is the kind of change I'll put in when it's ready.
>
> ...
>
> +# NOTE: This is also deprecated; you should provide a ProgressView instead.
>  class _BaseProgressBar(object):
>
> Same here, just wrap __init__:
>
>     @deprecated_method(deprecated_in((1, 17, 0)))
>     def __init__(self,
>
> Alternatively if you still need to do it from time to time, we've used a
> flag like "_internal=False" and then:
>
> if not _internal:
>  warn...

All the public concrete constructors are deprecated. You're right I
could and maybe should add an _internal flag so that any callers other
than those subclasses will warn.

>
>
> So other than
>
> 1) Deprecating things with comments rather than deprecated_in()

My cover letter wasn't very clear: these things are already actually
deprecated, it's just that the deprecation marks the constructor not
the class and I wanted it to be more obvious.

> I approve, but it needs fixing. I would vote "tweak" but I don't know
> how to do that.

There's a bug asking for a 'tweak' status. -
https://bugs.edge.launchpad.net/launchpad-code/+bug/373078

--
Martin <http://launchpad.net/~mbp/>