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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/387717-progress-bar-tty into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This stops progress bars being sent to stderr when it's not a file.
>
> (That's not quite the point of bug 387717, but it's still useful.)
>
> There are some more changes in here: ProgressTasks now talk directly to their ProgressView rather than to the UIFactory which makes them more easily testable in isolation, and may let us remove some forwarding methods in the UIFactory. (The factory still constructs the task so it can arrange to have notifications sent where ever it wants.)
>
> Some classes whose constructor is deprecated now have comments pointing this out.
>
> LINES, TERM and COLUMNS are set to specific values when running tests to reduce variation.
>

...

@@ -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?

...

+# 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...

So other than

1) Deprecating things with comments rather than deprecated_in()
2)
* Progress bars are now suppressed again when the environment variable
- - ``BZR_PROGRESS_BAR`` is set to ``none``.
- - (Martin Pool, #339385)
+ ``BZR_PROGRESS_BAR`` is set to ``none``, or when stderr is directed to
+ a terminal.
+ (Martin Pool, #339385, #387717)

^- And this having the wrong meaning.

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

Review Approve
  review: needs_fixing

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAko7mm4ACgkQJdeBCYSNAAPRKwCeJtFuuy6f7RbFEpw7fsNUubkQ
BMgAoICd00+tFOLRsG69P3PFNSGbYVBJ
=Kpqi
-----END PGP SIGNATURE-----