Patchwork [1,of,2] localrepo: document that __contains__() may raise LookupError

login
register
mail settings
Submitter Yuya Nishihara
Date May 25, 2017, 2:56 p.m.
Message ID <2dd7893c39fb0df46cfa.1495724191@mimosa>
Download mbox | patch
Permalink /patch/20899/
State Accepted
Headers show

Comments

Yuya Nishihara - May 25, 2017, 2:56 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1495721882 -32400
#      Thu May 25 23:18:02 2017 +0900
# Node ID 2dd7893c39fb0df46cfa6fbaa901f8da95beffda
# Parent  2b5953a49f1407f825d65b45986d213cb5c79203
localrepo: document that __contains__() may raise LookupError
via Mercurial-devel - May 27, 2017, 5:58 a.m.
On Thu, May 25, 2017 at 7:56 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1495721882 -32400
> #      Thu May 25 23:18:02 2017 +0900
> # Node ID 2dd7893c39fb0df46cfa6fbaa901f8da95beffda
> # Parent  2b5953a49f1407f825d65b45986d213cb5c79203
> localrepo: document that __contains__() may raise LookupError
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -573,6 +573,10 @@ class localrepository(object):
>          return context.changectx(self, changeid)
>
>      def __contains__(self, changeid):
> +        """True if the given changeid exists
> +
> +        error.LookupError is raised if an ambiguous node specified.

This seems surprising. Would it make sense to return True instead?
Would it be a lot of work to fix callers?

> +        """
>          try:
>              self[changeid]
>              return True
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - May 27, 2017, 1:20 p.m.
On Fri, 26 May 2017 22:58:21 -0700, Martin von Zweigbergk wrote:
> On Thu, May 25, 2017 at 7:56 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1495721882 -32400
> > #      Thu May 25 23:18:02 2017 +0900
> > # Node ID 2dd7893c39fb0df46cfa6fbaa901f8da95beffda
> > # Parent  2b5953a49f1407f825d65b45986d213cb5c79203
> > localrepo: document that __contains__() may raise LookupError
> >
> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > --- a/mercurial/localrepo.py
> > +++ b/mercurial/localrepo.py
> > @@ -573,6 +573,10 @@ class localrepository(object):
> >          return context.changectx(self, changeid)
> >
> >      def __contains__(self, changeid):
> > +        """True if the given changeid exists
> > +
> > +        error.LookupError is raised if an ambiguous node specified.
> 
> This seems surprising. Would it make sense to return True instead?
> Would it be a lot of work to fix callers?

Maybe no. The result is tri-state and it's up to callers whether an ambiguous
changeid should be considered True (=FoundSome) or False (=NotIdentified).
And we have pretty low test coverage.

% grep 'ambiguous identi' tests/*.t
tests/test-revset.t:  abort: 00changelog.i@2: ambiguous identifier!

We could add AmbiguousLookupError to make things slightly clearer, btw.
via Mercurial-devel - May 27, 2017, 9:29 p.m.
On Sat, May 27, 2017 at 6:20 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Fri, 26 May 2017 22:58:21 -0700, Martin von Zweigbergk wrote:
>> On Thu, May 25, 2017 at 7:56 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>> > # HG changeset patch
>> > # User Yuya Nishihara <yuya@tcha.org>
>> > # Date 1495721882 -32400
>> > #      Thu May 25 23:18:02 2017 +0900
>> > # Node ID 2dd7893c39fb0df46cfa6fbaa901f8da95beffda
>> > # Parent  2b5953a49f1407f825d65b45986d213cb5c79203
>> > localrepo: document that __contains__() may raise LookupError
>> >
>> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> > --- a/mercurial/localrepo.py
>> > +++ b/mercurial/localrepo.py
>> > @@ -573,6 +573,10 @@ class localrepository(object):
>> >          return context.changectx(self, changeid)
>> >
>> >      def __contains__(self, changeid):
>> > +        """True if the given changeid exists
>> > +
>> > +        error.LookupError is raised if an ambiguous node specified.
>>
>> This seems surprising. Would it make sense to return True instead?
>> Would it be a lot of work to fix callers?
>
> Maybe no. The result is tri-state and it's up to callers whether an ambiguous
> changeid should be considered True (=FoundSome) or False (=NotIdentified).
> And we have pretty low test coverage.
>
> % grep 'ambiguous identi' tests/*.t
> tests/test-revset.t:  abort: 00changelog.i@2: ambiguous identifier!

Yeah, it is probably best to leave as is. Queuing this, thanks.

>
> We could add AmbiguousLookupError to make things slightly clearer, btw.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -573,6 +573,10 @@  class localrepository(object):
         return context.changectx(self, changeid)
 
     def __contains__(self, changeid):
+        """True if the given changeid exists
+
+        error.LookupError is raised if an ambiguous node specified.
+        """
         try:
             self[changeid]
             return True