Patchwork [2,of,2,V3] error: make hintable exceptions reject unknown keyword arguments (API)

login
register
mail settings
Submitter Yuya Nishihara
Date July 11, 2016, 2:28 p.m.
Message ID <292f0845baa57737bc9b.1468247299@mimosa>
Download mbox | patch
Permalink /patch/15795/
State Accepted
Headers show

Comments

Yuya Nishihara - July 11, 2016, 2:28 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1468240802 -32400
#      Mon Jul 11 21:40:02 2016 +0900
# Node ID 292f0845baa57737bc9b8ce9d21d124dba157cc2
# Parent  5a15266e4b16eb9dba59fa561a2c3b6d3214db4b
error: make hintable exceptions reject unknown keyword arguments (API)

Previously they would accept any typos of the hint keyword.
Kostia Balytskyi - July 12, 2016, 9:39 a.m.
These 2 patches look good to me. Once they are accepted I will introduce MercurialException and replace Exception -> MercurialException here. My goal is not related to dispatch.runcommand(), but more to unshelve’s shelvedstate.load exception-catching, but I think it’s generally a good thing to do.

On 7/11/16, 3:28 PM, "Mercurial-devel on behalf of Yuya Nishihara" <mercurial-devel-bounces@mercurial-scm.org on behalf of yuya@tcha.org> wrote:

># HG changeset patch

># User Yuya Nishihara <yuya@tcha.org>

># Date 1468240802 -32400

>#      Mon Jul 11 21:40:02 2016 +0900

># Node ID 292f0845baa57737bc9b8ce9d21d124dba157cc2

># Parent  5a15266e4b16eb9dba59fa561a2c3b6d3214db4b

>error: make hintable exceptions reject unknown keyword arguments (API)

>

>Previously they would accept any typos of the hint keyword.

>

>diff --git a/mercurial/error.py b/mercurial/error.py

>--- a/mercurial/error.py

>+++ b/mercurial/error.py

>@@ -18,12 +18,12 @@ from __future__ import absolute_import

> 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.

>+    This should come first in the inheritance list to consume a hint and

>+    pass remaining arguments to the exception class.

>     """

>     def __init__(self, *args, **kw):

>-        super(Hint, self).__init__(*args)

>-        self.hint = kw.get('hint')

>+        self.hint = kw.pop('hint', None)

>+        super(Hint, self).__init__(*args, **kw)

> 

> class RevlogError(Hint, Exception):

>     pass

>diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py

>--- a/mercurial/subrepo.py

>+++ b/mercurial/subrepo.py

>@@ -56,9 +56,9 @@ def _getstorehashcachename(remotepath):

> class SubrepoAbort(error.Abort):

>     """Exception class used to avoid handling a subrepo error more than once"""

>     def __init__(self, *args, **kw):

>+        self.subrepo = kw.pop('subrepo', None)

>+        self.cause = kw.pop('cause', None)

>         error.Abort.__init__(self, *args, **kw)

>-        self.subrepo = kw.get('subrepo')

>-        self.cause = kw.get('cause')

> 

> def annotatesubrepoerror(func):

>     def decoratedmethod(self, *args, **kargs):

>_______________________________________________

>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=u1Nss-4AcSzhEtnaLykilc2S5_VePcD0L3v978GuwAY&s=WL4gb7tLHGfce7tTDtoJJ-ERGlP8-5tBIJS5xa4eSWE&e=
Yuya Nishihara - July 12, 2016, 1:11 p.m.
On Tue, 12 Jul 2016 09:39:25 +0000, Kostia Balytskyi wrote:
> These 2 patches look good to me. Once they are accepted I will introduce
> MercurialException and replace Exception -> MercurialException here. My goal
> is not related to dispatch.runcommand(), but more to unshelve’s
> shelvedstate.load exception-catching, but I think it’s generally a good
> thing to do.

I don't get the point why you prefer catching everything except
MercurialException over catching exceptions that can be raised by
int() and nodemod.bin(). IMHO, it's bad practice to suppress unknown
exceptions.
Kostia Balytskyi - July 12, 2016, 2:02 p.m.
On 7/12/16, 2:11 PM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:

>On Tue, 12 Jul 2016 09:39:25 +0000, Kostia Balytskyi wrote:

>> These 2 patches look good to me. Once they are accepted I will introduce

>> MercurialException and replace Exception -> MercurialException here. My goal

>> is not related to dispatch.runcommand(), but more to unshelve’s

>> shelvedstate.load exception-catching, but I think it’s generally a good

>> thing to do.

>

>I don't get the point why you prefer catching everything except

>MercurialException over catching exceptions that can be raised by

>int() and nodemod.bin(). IMHO, it's bad practice to suppress unknown

>exceptions.


I do want to catch exceptions that can be raised by int() and nodemod.bin(), but I want
to also be able to catch things like “unknown node” or “filtered node”. I also think that this
might be useful beyond unshelve usecase.
Yuya Nishihara - July 12, 2016, 2:40 p.m.
On Tue, 12 Jul 2016 14:02:27 +0000, Kostia Balytskyi wrote:
> On 7/12/16, 2:11 PM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
> 
> >On Tue, 12 Jul 2016 09:39:25 +0000, Kostia Balytskyi wrote:  
> >> These 2 patches look good to me. Once they are accepted I will introduce
> >> MercurialException and replace Exception -> MercurialException here. My goal
> >> is not related to dispatch.runcommand(), but more to unshelve’s
> >> shelvedstate.load exception-catching, but I think it’s generally a good
> >> thing to do.  
> >
> >I don't get the point why you prefer catching everything except
> >MercurialException over catching exceptions that can be raised by
> >int() and nodemod.bin(). IMHO, it's bad practice to suppress unknown
> >exceptions.  
> 
> I do want to catch exceptions that can be raised by int() and nodemod.bin(), but I want
> to also be able to catch things like “unknown node” or “filtered node”.

These two are RepoLookupError.

> I also think that this
> might be useful beyond unshelve usecase.

Yeah, I agree with that.
Augie Fackler - July 12, 2016, 4:44 p.m.
On Tue, Jul 12, 2016 at 09:39:25AM +0000, Kostia Balytskyi wrote:
> These 2 patches look good to me. Once they are accepted I will introduce MercurialException and replace Exception -> MercurialException here. My goal is not related to dispatch.runcommand(), but more to unshelve’s shelvedstate.load exception-catching, but I think it’s generally a good thing to do.

Queued these, thanks

>
> On 7/11/16, 3:28 PM, "Mercurial-devel on behalf of Yuya Nishihara" <mercurial-devel-bounces@mercurial-scm.org on behalf of yuya@tcha.org> wrote:
>
> ># HG changeset patch
> ># User Yuya Nishihara <yuya@tcha.org>
> ># Date 1468240802 -32400
> >#      Mon Jul 11 21:40:02 2016 +0900
> ># Node ID 292f0845baa57737bc9b8ce9d21d124dba157cc2
> ># Parent  5a15266e4b16eb9dba59fa561a2c3b6d3214db4b
> >error: make hintable exceptions reject unknown keyword arguments (API)
> >
> >Previously they would accept any typos of the hint keyword.
> >
> >diff --git a/mercurial/error.py b/mercurial/error.py
> >--- a/mercurial/error.py
> >+++ b/mercurial/error.py
> >@@ -18,12 +18,12 @@ from __future__ import absolute_import
> > 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.
> >+    This should come first in the inheritance list to consume a hint and
> >+    pass remaining arguments to the exception class.
> >     """
> >     def __init__(self, *args, **kw):
> >-        super(Hint, self).__init__(*args)
> >-        self.hint = kw.get('hint')
> >+        self.hint = kw.pop('hint', None)
> >+        super(Hint, self).__init__(*args, **kw)
> >
> > class RevlogError(Hint, Exception):
> >     pass
> >diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> >--- a/mercurial/subrepo.py
> >+++ b/mercurial/subrepo.py
> >@@ -56,9 +56,9 @@ def _getstorehashcachename(remotepath):
> > class SubrepoAbort(error.Abort):
> >     """Exception class used to avoid handling a subrepo error more than once"""
> >     def __init__(self, *args, **kw):
> >+        self.subrepo = kw.pop('subrepo', None)
> >+        self.cause = kw.pop('cause', None)
> >         error.Abort.__init__(self, *args, **kw)
> >-        self.subrepo = kw.get('subrepo')
> >-        self.cause = kw.get('cause')
> >
> > def annotatesubrepoerror(func):
> >     def decoratedmethod(self, *args, **kargs):
> >_______________________________________________
> >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=u1Nss-4AcSzhEtnaLykilc2S5_VePcD0L3v978GuwAY&s=WL4gb7tLHGfce7tTDtoJJ-ERGlP8-5tBIJS5xa4eSWE&e=
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/error.py b/mercurial/error.py
--- a/mercurial/error.py
+++ b/mercurial/error.py
@@ -18,12 +18,12 @@  from __future__ import absolute_import
 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.
+    This should come first in the inheritance list to consume a hint and
+    pass remaining arguments to the exception class.
     """
     def __init__(self, *args, **kw):
-        super(Hint, self).__init__(*args)
-        self.hint = kw.get('hint')
+        self.hint = kw.pop('hint', None)
+        super(Hint, self).__init__(*args, **kw)
 
 class RevlogError(Hint, Exception):
     pass
diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -56,9 +56,9 @@  def _getstorehashcachename(remotepath):
 class SubrepoAbort(error.Abort):
     """Exception class used to avoid handling a subrepo error more than once"""
     def __init__(self, *args, **kw):
+        self.subrepo = kw.pop('subrepo', None)
+        self.cause = kw.pop('cause', None)
         error.Abort.__init__(self, *args, **kw)
-        self.subrepo = kw.get('subrepo')
-        self.cause = kw.get('cause')
 
 def annotatesubrepoerror(func):
     def decoratedmethod(self, *args, **kargs):