[prev in list] [next in list] [prev in thread] [next in thread] 

List:       mercurial-devel
Subject:    Re: [PATCH 4 of 9 RFC] localrepo: add a method to return markers in anamespacee
From:       Sean Farley <sean.michael.farley () gmail ! com>
Date:       2014-03-31 19:30:02
Message-ID: m2ob0mcgjp.fsf () gmail ! com
[Download RAW message or body]


David Soria Parra <davidsp@fb.com> writes:

> Sean Farley <sean.michael.farley@gmail.com> writes:
>
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1396218396 18000
>> #      Sun Mar 30 17:26:36 2014 -0500
>> # Node ID 0c5c36fe392d9dfbc56ac466e312a62bdeb2bf93
>> # Parent  1bb668ecda482ee83f31d505c2094e6402668931
>> localrepo: add a method to return markers in a namespace
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -671,10 +671,37 @@ class localrepository(object):
>>          RFC: should we allow tags and bookmarks to be set this way?
>>          '''
>>          self._createmarkernamespace(namespace)
>>          self._markers[namespace][name] = node
>>  
>> +    def markers(self, namespace=None, name=None):
>> +        '''Return markers in the namespace, otherwise return all markers.
>> +
>> +        If namespace is None, this will return a dictionary of dictionaries.
>> +
>> +        If namespace is passed but name is None, this function will return a
>> +        dictionary.
>> +
>> +        If both namespace and name are passed, this will return a value.
>> +
>> +        RFC: should this allow namespace=None and name='foo' to find all 'foo'
>> +        no matter the namespace?
>> +        '''
>> +        if namespace is None:
>> +            # create a read-only copy that adds tags and bookmarks
>> +            allmarks = self._markers.copy()
>> +            allmarks["tag"] = self.tags()
>> +            allmarks["bookmark"] = self._bookmarks
>> +            return allmarks
>> +
>> +        self._createmarkernamespace(namespace)
>> +
>> +        if name is not None:
>> +            return self._markers[namespace][name]
>> +
>> +        return self._markers[namespace]
>> +
>
> I personally think this is a bit error prone as you can accidentally
> have a name = None when passing to markers and then you suddenly get a
> dictionary instead of an expected value. I would recommend to only
> return self._markers[namespace] so people can use it as
> self.markers(namespace).get(name, default).

That's a good point. I'll rework it as you suggest. Other points about
this patch series are the two scenarios:

Where can I inject to provide a custom namespace? Should we promote
wrapping _createmarkernamespace? Or load them during during reposetup?

Where can I inject to save this custom namespace? Following the logic
from local tags, an extension could wrap localrepo.mark and write out a
file each time. What if an extension wants to add 10 marks at once? I
can't imagine writing a file 10 times in a row is good.
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@selenic.com
http://selenic.com/mailman/listinfo/mercurial-devel
[prev in list] [next in list] [prev in thread] [next in thread]