[patch] show unexpected tracebacks to stderr
John Arbash Meinel
john at arbash-meinel.com
Thu Jun 15 15:00:38 BST 2006
Martin Pool wrote:
> bzr.log was a nice idea, but it's a bit unobvious.
>
> This patch changes the behaviour when an exception hits the top level to
> be this:
>
> - some cases like KeyboardInterrupt or EPIPE are handled specially
> - errors caused by a user problem (typo, bad url, etc) are reported
> briefly; in general we should only do this when we're confident
> that's really what happened.
> - everything else is assumed to be a bzr bug and reported to stderr
> with a traceback
>
> There are also a handful of cleanups of exceptions and tests for them.
>
> There is scope for more work in cleaning up existing error classes and
> tuning just which ones are reported as user errors, but I thought I'd
> send this up for +1 as it is.
>
>
I can see a lot of nice cleanup happening.
...
> @@ -337,9 +337,7 @@
> else:
> optname = a[2:]
> if optname not in cmd_options:
> - raise BzrOptionError('unknown long option %r for'
> - ' command %s' %
> - (a, command.name()))
> + raise BzrCommandError('unknown option "%s"' % a)
> else:
> shortopt = a[1:]
> if shortopt in Option.SHORT_OPTIONS:
> @@ -354,7 +352,7 @@
> if shortopt not in Option.SHORT_OPTIONS:
> # We didn't find the multi-character name, and we
> # didn't find the single char name
> - raise BzrError('unknown short option %r' % a)
> + raise BzrCommandError('unknown option "%s"' % a)
> optname = Option.SHORT_OPTIONS[shortopt].name
>
> if a[2:]:
> @@ -371,15 +369,12 @@
> # This option takes an argument, so pack it
> # into the array
> optarg = a[2:]
> -
> if optname not in cmd_options:
> - raise BzrOptionError('unknown short option %r for'
> - ' command %s' %
> - (shortopt, command.name()))
> + raise BzrCommandError('unknown option "%s"' % shortopt)
> if optname in opts:
> # XXX: Do we ever want to support this, e.g. for -r?
> if proc_aliasarg:
> - raise BzrError('repeated option %r' % a)
> + raise BzrCommandError('repeated option %r' % a)
> elif optname in alias_opts:
> # Replace what's in the alias with what's in the real
> # argument
Why did you get rid of BzrOptionError in favor of BzrCommandError?
...
v- PEP8 here
> -class BzrCommandError(BzrError):
> +class BzrCommandError(BzrNewError):
> + """Error from user command"""
> + is_user_error = True
> # Error from malformed user command
> # This is being misused as a generic exception
> # pleae subclass. RBC 20051030
> @@ -172,15 +195,14 @@
> # are not intended to be caught anyway. UI code need not subclass
> # BzrCommandError, and non-UI code should not throw a subclass of
> # BzrCommandError. ADHB 20051211
...
The '%r' was in case there was a Unicode path that could not be
displayed in the current encoding. You can chose how you want to handle
it. But is PathError a user_error or not?
These seem like hidden internal classes, that if one escapes, it should
be a bug.
>
> class NoSuchFile(PathError):
> - """No such file: %(path)r%(extra)s"""
> + """No such file: %(path)s%(extra)s"""
>
>
> class FileExists(PathError):
> - """File exists: %(path)r%(extra)s"""
> + """File exists: %(path)s%(extra)s"""
>
>
> class DirectoryNotEmpty(PathError):
> - """Directory not empty: %(path)r%(extra)s"""
> + """Directory not empty: %(path)s%(extra)s"""
>
>
> class ResourceBusy(PathError):
> - """Device or resource busy: %(path)r%(extra)s"""
> + """Device or resource busy: %(path)s%(extra)s"""
>
>
> class PermissionDenied(PathError):
> - """Permission denied: %(path)r%(extra)s"""
> + """Permission denied: %(path)s%(extra)s"""
>
>
> class PathNotChild(BzrNewError):
> - """Path %(path)r is not a child of path %(base)r%(extra)s"""
> + """Path %(path)s is not a child of path %(base)s%(extra)s"""
> +
> + is_user_error = False
> +
> def __init__(self, path, base, extra=None):
> BzrNewError.__init__(self)
> self.path = path
> @@ -244,7 +269,7 @@
>
v- PEP8 here as well
> class ObjectNotLocked(LockError):
> - """%(obj)r is not locked"""
> + """%(obj)s is not locked"""
> + is_user_error = False
> # this can indicate that any particular object is not locked; see also
> # LockNotHeld which means that a particular *lock* object is not held by
> # the caller -- perhaps they should be unified.
> @@ -366,7 +388,7 @@
>
v- Here you switch r => s, but then left another %r
> class LockBreakMismatch(LockError):
> - """Lock was released and re-acquired before being broken: %(lock)s: held by %(holder)r, wanted to break %(target)r"""
> + """Lock was released and re-acquired before being broken: %(lock)s: held by %(holder)s, wanted to break %(target)r"""
> def __init__(self, lock, holder, target):
> self.lock = lock
> self.holder = holder
> @@ -425,42 +447,38 @@
> """Commit refused because there are unknowns in the tree."""
>
>
> === modified file 'bzrlib/tests/test_source.py'
> --- bzrlib/tests/test_source.py
> +++ bzrlib/tests/test_source.py
> @@ -72,4 +72,4 @@
> # written. Note that this is an exact equality so that when the number
> # drops, it is not given a buffer but rather this test updated
> # immediately.
> - self.assertEqual(4, occurences)
> + self.assertEqual(3, occurences)
Good for us.
v- I thought we weren't doing 'Authors' statements.
> === modified file 'bzrlib/tests/test_trace.py'
> --- bzrlib/tests/test_trace.py
> +++ bzrlib/tests/test_trace.py
...
These seem like API changes that would need to be deprecated first.
> -
> -def log_exception(msg=None):
> - """Log the last exception to stderr and the trace file.
> -
> - The exception string representation is used as the error
> - summary, unless msg is given.
> - """
> - if msg:
> - error(msg)
> - else:
> - exc_str = format_exception_short(sys.exc_info())
> - error(exc_str)
> - log_exception_quietly()
> -
> -
> def log_exception_quietly():
> """Log the last exception to the trace file only.
>
Does this handle the case of commands that shouldn't show anything on
EPIPE? Probably that exception is caught in the decorator, so isn't
important here.
> +def report_exception(exc_info, err_file):
> + exc_type, exc_object, exc_tb = exc_info
> + if (isinstance(exc_object, IOError)
> + and getattr(exc_object, 'errno', None) == errno.EPIPE):
> + print >>err_file, "bzr: broken pipe"
> + elif isinstance(exc_object, KeyboardInterrupt):
> + print >>err_file, "bzr: interrupted"
> + elif getattr(exc_object, 'is_user_error', False):
> + report_user_error(exc_info, err_file)
> + elif isinstance(exc_object, (OSError, IOError)):
> + # Might be nice to catch all of these and show them as something more
> + # specific, but there are too many cases at the moment.
> + report_user_error(exc_info, err_file)
> + else:
> + report_bug(exc_info, err_file)
A couple small things, but +1 overall.
John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060615/3ef2ca92/attachment.pgp
More information about the bazaar
mailing list