Code review comment for lp:~mbp/bzr/remove-logging

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

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/remove-logging into lp:bzr.
>
> Requested reviews:
> Michael Hudson (mwhudson):
> bzr-core (bzr-core)
>
>
> This removes bzr's use of the Python logging module. Now all output either goes through bzrlib.trace to the log file (for developer-oriented information) or through Command.outf or UIFactory to the user (for regular interaction.)
>
> This may slightly improve startup time (or enable improving it); more importantly it reduces the proliferation of different methods so that they're easier to debug and test. (Exhibit A, the diff to test_plugins, removing about 20 lines of overhead.)
>
> This deprecates some APIs, but shouldn't break callers unless they're relying on bzrlib writing to logging. (Launchpad might?) But since we never did that very consistently, they're probably better off changing.
>
> * Add some developer documentation about how things are meant to work - previously discussed on the list on "[rfc] developer documentation on user interaction"
>
> * As a side-effect this causes more notice-type messages to go to stderr.
>
> * Remove some unused test support related to logging.
>
> * There were multiple names for equivalent methods in trace.py - some are now deprecated.
>
> * trace.py sends user-oriented messages to the UIFactory.
>
> Also some drive-by fixes while testing this:
>
> * Delete test that was redundant with UpgradeRecommendedTests.
>
> * Update some old tests from before tags were supported by default.
>

=== modified file 'Makefile'
- --- Makefile 2009-11-16 19:21:51 +0000
+++ Makefile 2009-11-17 08:05:40 +0000
@@ -39,7 +39,7 @@
 check: docs check-nodocs

 check-nodocs: extensions
- - $(PYTHON) -Werror -O ./bzr selftest -1v $(tests)
+ $(PYTHON) -Werror -O ./bzr selftest -1 --subunit $(tests)

^- This wasn't mentioned. Are we officially switching back? Doesn't the
pipeline need to be updated since "selftest --subunit" will no longer
return a non-0 return code? (At least from my understanding of the
conversation, Robert said you need the failure to be determined from the
later processing stages.)

Also, if we are switching to --subunit, I think it is reasonable to get
rid of -1. As you can easily parse the subunit output to just get the
failures, which means in 1 round trip you can fix everything that failed.

...

=== modified file 'NEWS'
- --- NEWS 2009-11-17 04:03:45 +0000
+++ NEWS 2009-11-17 08:05:40 +0000
@@ -418,6 +418,9 @@
 * PreviewTree behaves correctly when get_file_mtime is invoked on an
unmodified
   file. (Aaron Bentley, #251532)

+* PreviewTree behaves correctly when get_file_mtime is invoked on an
unmodified
+ file. (Aaron Bentley, #251532)
+
 * Registry objects should not use iteritems() when asked to use items().
   (Vincent Ladeuil, #430510)

^- Something doesn't look right here...

In general, all the NEWS items seem a bit confused. Are they just going
into the wrong section? (Unfortunately NEWS means you can't really use
'daggy' fixes, because then the merge into NEWS is always wrong, as it
has to be done with the "latest" version.)

Looking further, the merge seems very confused about what you've done.
Is it possible for you to clean that up and resubmit?

 review: needs_information

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

iEYEARECAAYFAksCvwwACgkQJdeBCYSNAAMqKQCgqM2xZstAXDnwdCII6kMS+G9J
raEAn2xGc8o6d+Y48H8hFsX/GhUslJm4
=5IAq
-----END PGP SIGNATURE-----