Patchwork [V2] error: make HintException a mix-in class not derived from BaseException (API)

login
register
mail settings
Submitter Yuya Nishihara
Date July 9, 2016, 6:20 a.m.
Message ID <bf279db10eb66c70322b.1468045236@mimosa>
Download mbox | patch
Permalink /patch/15780/
State Superseded
Headers show

Comments

Yuya Nishihara - July 9, 2016, 6:20 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1468042110 -32400
#      Sat Jul 09 14:28:30 2016 +0900
# Node ID bf279db10eb66c70322b062ef1818ddfb06e6daa
# Parent  b4d117cee636be8a566f56e84d4b351a736a1299
error: make HintException a mix-in class not derived from BaseException (API)

HintException is unrelated to the hierarchy of errors. It is an implementation
detail whether a class inherits from HintException or not, a sort of "private
inheritance" in C++.

New Hint isn't an exception class, which prevents catching error by its type:

    try:
        dosomething()
    except error.Hint:
        pass

Unfortunately, this passes on PyPy 5.3.1, but not on Python 2, and raises more
detailed TypeError on Python 3.
Augie Fackler - July 10, 2016, 6:35 p.m.
On Sat, Jul 09, 2016 at 03:20:36PM +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1468042110 -32400
> #      Sat Jul 09 14:28:30 2016 +0900
> # Node ID bf279db10eb66c70322b062ef1818ddfb06e6daa
> # Parent  b4d117cee636be8a566f56e84d4b351a736a1299
> error: make HintException a mix-in class not derived from BaseException (API)
>
> HintException is unrelated to the hierarchy of errors. It is an implementation
> detail whether a class inherits from HintException or not, a sort of "private
> inheritance" in C++.
>
> New Hint isn't an exception class, which prevents catching error by its type:
>
>     try:
>         dosomething()
>     except error.Hint:
>         pass
>
> Unfortunately, this passes on PyPy 5.3.1, but not on Python 2, and raises more
> detailed TypeError on Python 3.
>
> diff --git a/mercurial/error.py b/mercurial/error.py
> --- a/mercurial/error.py
> +++ b/mercurial/error.py
> @@ -15,12 +15,17 @@ from __future__ import absolute_import
>
>  # Do not import anything here, please
>
> -class HintException(Exception):
> +class Hint(object):
> +    """Mix-in to provide a hint of an error
> +
> +    This should come first in the inheritance list to consume **kw and pass
> +    only *args to the exception class.
> +    """
>      def __init__(self, *args, **kw):
> -        Exception.__init__(self, *args)
> +        super(Hint, self).__init__(*args)
>          self.hint = kw.get('hint')

should we do kw.pop('hint', None) and then forward **kw?

>
> -class RevlogError(HintException):
> +class RevlogError(Hint, Exception):
>      pass
>
>  class FilteredIndexError(IndexError):
> @@ -50,10 +55,10 @@ class ManifestLookupError(LookupError):
>  class CommandError(Exception):
>      """Exception raised on errors in parsing the command line."""
>
> -class InterventionRequired(HintException):
> +class InterventionRequired(Hint, Exception):
>      """Exception raised when a command requires human intervention."""
>
> -class Abort(HintException):
> +class Abort(Hint, Exception):
>      """Raised if a command needs to print an error and exit."""
>
>  class HookLoadError(Abort):
> @@ -87,10 +92,10 @@ class ResponseExpected(Abort):
>          from .i18n import _
>          Abort.__init__(self, _('response expected'))
>
> -class OutOfBandError(HintException):
> +class OutOfBandError(Hint, Exception):
>      """Exception raised when a remote repo reports failure"""
>
> -class ParseError(HintException):
> +class ParseError(Hint, Exception):
>      """Raised when parsing config files and {rev,file}sets (msg[, pos])"""
>
>  class UnknownIdentifier(ParseError):
> @@ -102,7 +107,7 @@ class UnknownIdentifier(ParseError):
>          self.function = function
>          self.symbols = symbols
>
> -class RepoError(HintException):
> +class RepoError(Hint, Exception):
>      pass
>
>  class RepoLookupError(RepoError):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Kostia Balytskyi - July 11, 2016, 5:23 a.m.
I would also argue in favor of MercurialException class to allow differentiation between our exceptions and all other ones.


-------- Original message --------
From: Augie Fackler <raf@durin42.com>
Date: 10/07/2016 20:36 (GMT+01:00)
To: Yuya Nishihara <yuya@tcha.org>
Cc: mercurial-devel@mercurial-scm.org
Subject: Re: [PATCH V2] error: make HintException a mix-in class not derived from BaseException (API)

On Sat, Jul 09, 2016 at 03:20:36PM +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1468042110 -32400
> #      Sat Jul 09 14:28:30 2016 +0900
> # Node ID bf279db10eb66c70322b062ef1818ddfb06e6daa
> # Parent  b4d117cee636be8a566f56e84d4b351a736a1299
> error: make HintException a mix-in class not derived from BaseException (API)
>
> HintException is unrelated to the hierarchy of errors. It is an implementation
> detail whether a class inherits from HintException or not, a sort of "private
> inheritance" in C++.
>
> New Hint isn't an exception class, which prevents catching error by its type:
>
>     try:
>         dosomething()
>     except error.Hint:
>         pass
>
> Unfortunately, this passes on PyPy 5.3.1, but not on Python 2, and raises more
> detailed TypeError on Python 3.
>
> diff --git a/mercurial/error.py b/mercurial/error.py
> --- a/mercurial/error.py
> +++ b/mercurial/error.py
> @@ -15,12 +15,17 @@ from __future__ import absolute_import
>
>  # Do not import anything here, please
>
> -class HintException(Exception):
> +class Hint(object):
> +    """Mix-in to provide a hint of an error
> +
> +    This should come first in the inheritance list to consume **kw and pass
> +    only *args to the exception class.
> +    """
>      def __init__(self, *args, **kw):
> -        Exception.__init__(self, *args)
> +        super(Hint, self).__init__(*args)
>          self.hint = kw.get('hint')

should we do kw.pop('hint', None) and then forward **kw?

>
> -class RevlogError(HintException):
> +class RevlogError(Hint, Exception):
>      pass
>
>  class FilteredIndexError(IndexError):
> @@ -50,10 +55,10 @@ class ManifestLookupError(LookupError):
>  class CommandError(Exception):
>      """Exception raised on errors in parsing the command line."""
>
> -class InterventionRequired(HintException):
> +class InterventionRequired(Hint, Exception):
>      """Exception raised when a command requires human intervention."""
>
> -class Abort(HintException):
> +class Abort(Hint, Exception):
>      """Raised if a command needs to print an error and exit."""
>
>  class HookLoadError(Abort):
> @@ -87,10 +92,10 @@ class ResponseExpected(Abort):
>          from .i18n import _
>          Abort.__init__(self, _('response expected'))
>
> -class OutOfBandError(HintException):
> +class OutOfBandError(Hint, Exception):
>      """Exception raised when a remote repo reports failure"""
>
> -class ParseError(HintException):
> +class ParseError(Hint, Exception):
>      """Raised when parsing config files and {rev,file}sets (msg[, pos])"""
>
>  class UnknownIdentifier(ParseError):
> @@ -102,7 +107,7 @@ class UnknownIdentifier(ParseError):
>          self.function = function
>          self.symbols = symbols
>
> -class RepoError(HintException):
> +class RepoError(Hint, Exception):
>      pass
>
>  class RepoLookupError(RepoError):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Pp-gQYFgs4tKlSFPF5kfCw&m=gH42mpMAY9pB4Ok1rt9tBazPXiw6uw3RPUi4Z1cjk60&s=2_2nXaJTF4i9-d9uWpdkF_axE51iJpp5i-JYam6I8ps&e=
Yuya Nishihara - July 11, 2016, 12:54 p.m.
On Mon, 11 Jul 2016 05:23:09 +0000, Kostia Balytskyi wrote:
> I would also argue in favor of MercurialException class to allow
> differentiation between our exceptions and all other ones.

That might be somewhat useful to eliminate "except Exception:" at
dispatch.runcommand(). But we would still have to catch IOError,
KeyboardInterrupt, etc. as well.

> > +class Hint(object):
> > +    """Mix-in to provide a hint of an error
> > +
> > +    This should come first in the inheritance list to consume **kw and pass
> > +    only *args to the exception class.
> > +    """
> >      def __init__(self, *args, **kw):
> > -        Exception.__init__(self, *args)
> > +        super(Hint, self).__init__(*args)
> >          self.hint = kw.get('hint')
> 
> should we do kw.pop('hint', None) and then forward **kw?

Sure, send V3.

Patch

diff --git a/mercurial/error.py b/mercurial/error.py
--- a/mercurial/error.py
+++ b/mercurial/error.py
@@ -15,12 +15,17 @@  from __future__ import absolute_import
 
 # Do not import anything here, please
 
-class HintException(Exception):
+class Hint(object):
+    """Mix-in to provide a hint of an error
+
+    This should come first in the inheritance list to consume **kw and pass
+    only *args to the exception class.
+    """
     def __init__(self, *args, **kw):
-        Exception.__init__(self, *args)
+        super(Hint, self).__init__(*args)
         self.hint = kw.get('hint')
 
-class RevlogError(HintException):
+class RevlogError(Hint, Exception):
     pass
 
 class FilteredIndexError(IndexError):
@@ -50,10 +55,10 @@  class ManifestLookupError(LookupError):
 class CommandError(Exception):
     """Exception raised on errors in parsing the command line."""
 
-class InterventionRequired(HintException):
+class InterventionRequired(Hint, Exception):
     """Exception raised when a command requires human intervention."""
 
-class Abort(HintException):
+class Abort(Hint, Exception):
     """Raised if a command needs to print an error and exit."""
 
 class HookLoadError(Abort):
@@ -87,10 +92,10 @@  class ResponseExpected(Abort):
         from .i18n import _
         Abort.__init__(self, _('response expected'))
 
-class OutOfBandError(HintException):
+class OutOfBandError(Hint, Exception):
     """Exception raised when a remote repo reports failure"""
 
-class ParseError(HintException):
+class ParseError(Hint, Exception):
     """Raised when parsing config files and {rev,file}sets (msg[, pos])"""
 
 class UnknownIdentifier(ParseError):
@@ -102,7 +107,7 @@  class UnknownIdentifier(ParseError):
         self.function = function
         self.symbols = symbols
 
-class RepoError(HintException):
+class RepoError(Hint, Exception):
     pass
 
 class RepoLookupError(RepoError):