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

[Martin von Zweigbergk]

On Sat, May 27, 2017 at 6:20 AM, Yuya Nishihara <yuya at 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 at tcha.org> wrote:
>> > # HG changeset patch
>> > # User Yuya Nishihara <yuya at 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 at 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.