| Submitter | Sean Farley |
|---|---|
| Date | March 30, 2014, 11:09 p.m. |
| Message ID | <0c5c36fe392d9dfbc56a.1396220942@laptop.local> |
| Download | mbox | patch |
| Permalink | /patch/4151/ |
| State | Deferred |
| Headers | show |
Comments
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).
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.
On Mon, 2014-03-31 at 17:41 +0000, David Soria Parra wrote: > 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. This is a bit too polymorphic an API: it has three return types (dict of dicts, dict, and value).
Matt Mackall <mpm@selenic.com> writes: > On Mon, 2014-03-31 at 17:41 +0000, David Soria Parra wrote: >> 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. > > This is a bit too polymorphic an API: it has three return types (dict of > dicts, dict, and value). Agreed. What about the name, though?
On Wed, 2014-04-02 at 12:01 -0500, Sean Farley wrote: > Matt Mackall <mpm@selenic.com> writes: > > > On Mon, 2014-03-31 at 17:41 +0000, David Soria Parra wrote: > >> 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. > > > > This is a bit too polymorphic an API: it has three return types (dict of > > dicts, dict, and value). > > Agreed. What about the name, though? If we agree that the shape of this function is: no args -> dictionary of name, hash pairs ..it's fine. But then we have questions about what to do with collisions. Perhaps all we really need is marked() -> set of nodes that have some form of marker.
Matt Mackall <mpm@selenic.com> writes: > On Wed, 2014-04-02 at 12:01 -0500, Sean Farley wrote: >> Matt Mackall <mpm@selenic.com> writes: >> >> > On Mon, 2014-03-31 at 17:41 +0000, David Soria Parra wrote: >> >> 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. >> > >> > This is a bit too polymorphic an API: it has three return types (dict of >> > dicts, dict, and value). >> >> Agreed. What about the name, though? > > If we agree that the shape of this function is: > > no args -> dictionary of name, hash pairs > > ..it's fine. But then we have questions about what to do with > collisions. Perhaps all we really need is marked() -> set of nodes that > have some form of marker. Sorry, I didn't clarify what I meant. It seems that 'marker' is not a good name because there is a clash with 'obsolete marker'. In another thread, I present the word 'label': http://www.selenic.com/pipermail/mercurial-devel/2014-March/057599.html
On Wed, 2014-04-02 at 16:20 -0500, Sean Farley wrote: > Matt Mackall <mpm@selenic.com> writes: > > > On Wed, 2014-04-02 at 12:01 -0500, Sean Farley wrote: > >> Matt Mackall <mpm@selenic.com> writes: > >> > >> > On Mon, 2014-03-31 at 17:41 +0000, David Soria Parra wrote: > >> >> 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. > >> > > >> > This is a bit too polymorphic an API: it has three return types (dict of > >> > dicts, dict, and value). > >> > >> Agreed. What about the name, though? > > > > If we agree that the shape of this function is: > > > > no args -> dictionary of name, hash pairs > > > > ..it's fine. But then we have questions about what to do with > > collisions. Perhaps all we really need is marked() -> set of nodes that > > have some form of marker. > > Sorry, I didn't clarify what I meant. It seems that 'marker' is not a > good name because there is a clash with 'obsolete marker'. In another > thread, I present the word 'label': > > http://www.selenic.com/pipermail/mercurial-devel/2014-March/057599.html Either is fine. I've suggested label in the past.
Patch
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] + def branchmap(self): '''returns a dictionary {branch: [branchheads]} with branchheads ordered by increasing revision number''' branchmap.updatecache(self) return self._branchcaches[self.filtername]