Code review comment for lp:~mbp/bzr/error-reporting

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

On 3/31/2011 6:49 AM, Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/error-reporting into lp:bzr with lp:~mbp/bzr/assert-doctest as a prerequisite.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #716389 in Bazaar: "In-terminal crash report is far too long, containing full `bzr plugins` output"
> https://bugs.launchpad.net/bzr/+bug/716389
>
> For more details, see:
> https://code.launchpad.net/~mbp/bzr/error-reporting/+merge/55677
>
> Per bug 716389 and previous feedback, this now shows just a concisely list of plugins in the non-apport crash message
>

Right now plugin.describe_plugins() uses:
  plugin = loaded_plugins[name]
  version = plugin.__version__
  if version == 'unknown':
      version = ''

We might want to do that for the formatting, so you don't get:
bzrtools[unknown], qbzr[1.2.3]

I don't have strong feelings on it, though. It might be good to report
unknown...

This is what the formatting looks like here (I have a fair number of
'unknown' version plugins):

bzr 2.4.0dev2 on python 2.6.4 (Windows-Vista-6.0.6002-SP2)
arguments: ['C:/Users/jameinel/dev/bzr/jam-integration/bzr', 'assert-fail']
plugins: bash_completion[2.4.0dev2], bzrtools[2.1b1],
    changelog_merge[2.4.0dev2], check_chk[unknown], colo[0.2.1dev],
    dummy_request[unknown], dump[unknown], email[unknown],
explorer[1.1.0dev],
    fastimport[0.9.0dev], file_locking[0.1.0dev], gimmepdb[unknown],
    grep[0.2.0], in_ancestry[unknown], launchpad[2.4.0dev2],
    lock_test[0.1.0dev], loggerhead[1.18.0], loom[2.2.1dev],
    netrc_credential_store[2.4.0dev2], news_merge[2.4.0dev2],
    per_file_graph[unknown], pqm[1.4.0dev], pybloom[unknown],
    qbzr[0.20.0dev1], rebase[0.4.3dev], register[unknown],
    repodetails[1.9.0dev], search[1.7.0dev], start[unknown],
stats[0.1.0dev],
    test_groupcompress[unknown], time_iter_changes[unknown],
    track_wow_renames[0.0.1dev], update_copyright[0.1.0dev],
vimdiff[unknown],
    weave_fmt[unknown]
encoding: 'cp1252', fsenc: 'mbcs', lang: 'C.UTF-8'

...

=== modified file 'bzrlib/tests/__init__.py'
- --- bzrlib/tests/__init__.py 2011-03-31 04:48:22 +0000
+++ bzrlib/tests/__init__.py 2011-03-31 04:48:30 +0000
@@ -1465,7 +1465,7 @@
         checker = doctest.OutputChecker()
         if optionflags is None:
             # XXX: More here by default?
- - optionflags = doctest.ELLIPSIS
+ optionflags = doctest.ELLIPSIS | doctest.REPORT_UDIFF
         if not checker.check_output(expected, actual, optionflags):
             # The Doctest api insists on an internal object here but
doesn't
             # really need it; just pretend.

^- Looks like this was meant to be part of the earlier patch.

test_crash does pass here on a Windows machine. So I'm happy that the
doctest matcher worked for what you wanted.

I do think you could have crafted some sort of multiline regex to get
the same results as the doctest matcher. I suppose escaping '.' is a bit
ugly (no worse than <BLANKLINE>, IMO, and '.*' is shorter than '...')

You do get much better error messages, though. Since regexes tend to say
"no, didn't match" without any context.

Anyway, I do think this is an improvement over the big multi-line plugin
info block.

 merge: approve

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

iEYEARECAAYFAk2UDVwACgkQJdeBCYSNAAMfTACg2i/st6NH4GZOwBxfLRgrKbAv
OY4An2Hgm9kSdkWWMnM8YfOc3hmDXYmL
=R9KT
-----END PGP SIGNATURE-----