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

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/progress-output into lp:bzr.
> 
>     Requested reviews:
>     bzr-core (bzr-core)
> 
> 
> This conceptually relates to <https://code.edge.launchpad.net/~mbp/bzr/remove-logging/+merge/14942> but doesn't technically depend on it.
>  
> +* New API ``ui_factory.make_output_stream`` to be used for sending bulk
> +  (rather than user-interaction) data to stdout.  This automatically
> +  coordinates with progress bars or other terminal activity, and can be
> +  overridden by GUIs.
> +  (Martin Pool)
> +
> 
> The idea is to 
> 
>  * Allow us to show transport activity whenever it happens, without clobbering the output of eg log - this doesn't actually restore this behaviour, but it gets ready for it.
> 
>  * Move away from Command.outf, which assumes that it's only the command layer that knows about stdout.  In fact, it's generally a layer or two down, and passing what's really a global is poor.
> 
>  * Also avoid code defaulting directly to sys.stdout.
> 
>  * In particular, put the unicode-handling features of outf into a more reusable place.
> 
>  * Give GUIs a way to capture bulk output of application-level commands to show in a window.
> 
> Command.outf still works and redirects into this.
> 

	
+    def test_make_output_stream(self):
+        # at the moment this is only implemented on text uis; i'm not sure
+        # what it should do elsewhere
+        try:
+            output_stream = self.factory.make_output_stream()
+        except NotImplementedError, e:
+            raise tests.TestSkipped(str(e))
+        output_stream.write('hello!')
+

^- Since Command._setup_outf is now relying on "make_output_stream" are
they allowed to not implement it?



+class TestTextUIOutputStream(TestCase):
+    """Tests for output stream that synchronizes with progress bar."""
+
+    def test_output_clears_terminal(self):
+        stdout = StringIO()
+        stderr = StringIO()
+        clear_calls = []
+
+        uif = TextUIFactory(None, stdout, stderr)
+        uif.clear_term = lambda: clear_calls.append('clear')
+
+        stream = TextUIOutputStream(uif, uif.stdout)
+        stream.write("Hello world!\n")
+        stream.write("there's more...\n")
+        stream.writelines(["1\n", "2\n", "3\n"])
+
+        self.assertEqual(stdout.getvalue(),
+            "Hello world!\n"
+            "there's more...\n"
+            "1\n2\n3\n")
+        self.assertEqual(['clear', 'clear', 'clear'],
+            clear_calls)
+
+        stream.flush()
+
+

^- This seems like a reasonable start. Though I personally am much more
interested in what happens if you pass Unicode bytes to this stream.
Especially in the "bzr log -p" or "bzr diff" case, which tends to mix
Unicode bytes with raw content from files.

I realize you do have an 'encoding_type' flag...


+        # For whatever reason codecs.getwriter() does not advertise its
encoding
+        # it just returns the encoding of the wrapped file, which is
completely
+        # bogus. So set the attribute, so we can find the correct
encoding later.
+        out_stream = self._make_output_stream_explicit(encoding,
encoding_type)
+        if not getattr(out_stream, 'encoding', None):
+            out_stream.encoding = encoding
+

^- The comment doesn't fit the action. It says that "w =
codecs.getwriter()" *does* have an 'encoding' attribute, but it returns
the value of the wrapped object. IOW

w = codecs.getwriter('utf-8', sys.stdout)
w.encoding == sys.stdout.encoding


rather than
w.encoding == 'utf-8'

I may be misunderstanding something, though. And if this was the code
that was originally present, than it makes sense to preserve the behavior.


I think the layering is nice. I'm a bit worried about having self.outf
*and* a bunch of library functions that then grab their own codec
wrapped stdout, and not having the code paths coordinate.

I also think this would be a reasonable time to change the api in a
larger way. Namely, have the wrapped object have:

 self.write_bytes('+diff content')
 self.write_unicode(u'Author:جوجو')

We could do that in addition to 'self.write' if we wanted to preserve
compatibility.

If we are going to do it, then I think it would be reasonable to create
an AbstractBaseClass of "OutputStream" that defines what apis should be
implemented.

So I'm marking this as "needs_information" because I'd like to have a
conversation about what you're thinking. Conceptually the code is fine,
and should be "approve"

 review: needs_information


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

iEYEARECAAYFAksCwYwACgkQJdeBCYSNAAP66ACfTJXnb+XFWV7f3bTa4ipUKmW1
rFsAni4FckHnOxFi4n/xmE/AoaCzwHbz
=cveG
-----END PGP SIGNATURE-----