[patch] [0.12] remove BzrNewError, cleanup exceptions and users

[John Arbash Meinel]

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

Martin Pool wrote:
> This cleans up the probably-mistaken BzrNewError behaviour of using error
> class docstrings as their format string.  Instead errors can define a _fmt
> attribute with the same meaning.  The docstring is now reserved for its
> regular purpose of documentation for programmers.  This behaviour is added to
> BzrError.  BzrNewError is left in place for compatibility but no builtin
> errors use it anymore and it gives a deprecation warning on construction.

I think this is a really nice cleanup,
...


v- I think we want to avoid calling things 'user errors'. Sort of like
"the customer is always right", while it may be something that the user
specifically did incorrectly, (supplying a path that doesn't exist, etc)

In general, we only distinguish them as something which is really a bug,
versus stuff that is just something like a bad path passed by the user.
What if we turn the logic around and use 'internal_error'?


> +Errors are handled through Python exceptions.
> +
> +We broadly classify errors as either being either the user's fault or our
> +fault.  If we think it's our fault, we show a backtrace, an invitation to
> +report the bug, and possibly other details.  This is the default for
> +errors that aren't specifically recognized as being caused by a user
> +error.
> +
> +User errors are things such as referring to a file or location that
> +doesn't exist.  The user could potentially fix them themselves, so they
> +get a short but helpful message.  User errors either have the
> +``is_user_error`` property set, or are of particular classes such as
> +KeyboardInterrupt.
> +
> +Many errors originate as "environmental errors" which are raised by Python
> +or builtin libraries -- for example IOError.  These are treated as being
> +our fault, unless they're caught in a particular tight scope where we know
> +that they indicate a user errors.  For example if the repository format
> +is not found, the user probably gave the wrong path or URL.  But if one of
> +the files inside the repository is not found, then it's our fault --
> +either there's a bug in bzr, or something complicated has gone wrong in
> +the environment that means one internal file was deleted.
> +
> +Many errors are defined in ``bzrlib/errors.py`` but it's OK for new errors
> +to be added near the place where they are used.
> +
> +Exceptions are formatted for the user by conversion to a string
> +(eventually calling their ``__str__`` method.)  As a convenience the
> +``._fmt`` member can be used as a template which will be mapped to the
> +error's instance dict.
> +
> +New exception classes should be defined when callers might want to catch
> +that exception specifically, or when it needs a substantially different
> +format string.
> +
> +Exception strings should start with a capital letter and should not have a
> +final fullstop.
> +
>  
>  
>  Jargon
> 
>  
>  
>  class BzrError(StandardError):
> +    """
> +    Base class for errors raised by bzrlib.
> +
> +    :cvar is_user_error: True if this is a user error and should be 
> +    displayed concisely; false if this is bzr's fault and it should 
> +    give a traceback.
> +

v- I think you want this to be :cvar _fmt:
> +    :cvar fmt: Format string to display the error; this is expanded
> +    by the instance's dict.
> +    """
>      
>      is_user_error = True
>  
> +    def __init__(self, msg=None, **kwds):
> +        """Construct a new BzrError.
> +
> +        There are two alternative forms for constructing these objects.
> +        Either a preformatted string may be passed, or a set of named
> +        arguments can be given.  The first is for generic "user" errors which
> +        are not intended to be caught and so do not need a specific subclass.
> +        The second case is for use with subclasses that provide a _fmt format
> +        string to print the arguments.  
> +
> +        Keyword arguments are taken as parameters to the error, which can 
> +        be inserted into the format string template.  It's recommended 
> +        that subclasses override the __init__ method to require specific 
> +        parameters.
> +

v- Did you want this to actually be deprecated? You're earlier message
made it sound like it was directly supported.
> +        :param msg: Deprecated parameter for passing a format string 
> +        when constructing.
> +        """
> +        StandardError.__init__(self)
> +        if msg is not None:
> +            ## symbol_versioning.warn(
> +            ##         'constructing a BzrError from a string was deprecated '
> +            ##         'in bzr 0.12; please raise a specific subclass instead',
> +            ##         DeprecationWarning,
> +            ##         stacklevel=2)
> +            self._str = msg
> +        else:
> +            self._str = None
> +            for key, value in kwds.items():
> +                setattr(self, key, value)
> +
>      def __str__(self):
> -        # XXX: Should we show the exception class in 
> -        # exceptions that don't provide their own message?  
> -        # maybe it should be done at a higher level
> -        ## n = self.__class__.__name__ + ': '
> -        n = ''
> -        if len(self.args) == 1:
> -            return str(self.args[0])
> -        elif len(self.args) == 2:
> -            # further explanation or suggestions
> -            try:
> -                return n + '\n  '.join([self.args[0]] + self.args[1])
> -            except TypeError:
> -                return n + "%r" % self
> -        else:
> -            return n + `self.args`

v- You haven't made any comment about 'self._str' anywhere else.
> +        s = getattr(self, '_str', None)
> +        if s is not None:
> +            # contains a preformatted message; must be cast to plain str
> +            return str(s)
> +        try:
> +            # __str__() should always return a 'str' object
> +            # never a 'unicode' object.
> +            s = self._fmt % self.__dict__
> +            if isinstance(s, unicode):
> +                return s.encode('utf8')
> +            return s
> +        except (AttributeError, TypeError, NameError, ValueError, KeyError), e:
> +            return 'Unprintable exception %s: dict=%r, fmt=%r, error=%s' \
> +                % (self.__class__.__name__,
> +                   self.__dict__,
> +                   getattr(self, '_fmt', None),
> +                   str(e))

^- This also assumes that '_fmt' always exists. I would tend to do:

fmt = getattr(self, '_fmt', None)
if fmt is None:
  DEPRECATED
  fmt = getattr(self, '__doc__')
  if fmt is BzrError.__doc__:
    UNPRINTABLE

That way, plugins, etc can still have Errors that have only doc strings.

...



v-- This looks like you open with a single quote, but close with a
triple quote. What is even weirder is that it seems to work just fine. I
assume it gets parsed as "foo" "". Regardless, we should only use a
single closing quote. But this one needs to get fixed up in other ways
as well, so I'm not set on doing that right now.

>  class BranchExistsWithoutWorkingTree(PathError):
> -    """Directory contains a branch, but no working tree \
> +
> +    _fmt = "Directory contains a branch, but no working tree \
>  (use bzr checkout if you wish to build a working tree): %(path)s"""
>  

...

v- These look like it needs another newline.

>  class ReadOnlyObjectDirtiedError(ReadOnlyError):
> -    """Cannot change object %(obj)r in read only transaction"""
> +
> +    _fmt = "Cannot change object %(obj)r in read only transaction"
>      def __init__(self, obj):
>          self.obj = obj
>  
>  
>  class UnlockableTransport(LockError):
> -    """Cannot lock: transport is read only: %(transport)s"""
> +
> +    _fmt = "Cannot lock: transport is read only: %(transport)s"
>      def __init__(self, transport):
>          self.transport = transport
>  

...

v- If we are here, we probably want to deprecate passing kwargs to the
base constructor, and instead have the child errors set 'self.var = var'
instead. At least, I know Robert prefers that form. I actually prefer
passing it to the base, but I'm not as set on that as Robert seems to be.

It seems that having the thing which does the formatting also manage the
values is less magic than expecting the side effect of setting it in the
child having it effect the base-level __str__ formatting.

But that also means that errors which are inherited from should take
'**kwds' and pass it up to the base.

>  class NoCommonRoot(BzrError):
> +
> +    _fmt = "Revisions are not derived from the same root: " \
> +           "%(revision_a)s %(revision_b)s."
> +
>      def __init__(self, revision_a, revision_b):
> -        msg = "Revisions are not derived from the same root: %s %s." \
> -            % (revision_a, revision_b) 
> -        BzrError.__init__(self, msg)
> -
> +        BzrError.__init__(self, revision_a=revision_a, revision_b=revision_b)
>  
>  

...

Originally I thought that this shouldn't go into 0.12 after the freeze,
but with a few small changes it seems mechanical enough. Though I guess
since it deprecates BzrNewError, and should deprecate __doc__ (right now
you just ignore it, and would fail if a plugin defined an error with no
class._fmt member).

So I think I'd like to see a few tweaks (deprecate __doc__ rather than
ignore it, and a few whitespace cleanups still). And probably it should
wait until next week.

John
=:->


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

iD8DBQFFM2J8JdeBCYSNAAMRAvSHAJ9wjv9o1wfCgDZ5teYa0Y0p1bP7BwCeLa2k
3kSahTPdhePFsLG3eusWBDc=
=pj7T
-----END PGP SIGNATURE-----