| Submitter | Katsunori FUJIWARA |
|---|---|
| Date | Dec. 29, 2015, 3:08 p.m. |
| Message ID | <0aa9cf12fc6ce052b0f2.1451401731@feefifofum> |
| Download | mbox | patch |
| Permalink | /patch/12401/ |
| State | Accepted |
| Headers | show |
Comments
On Wed, 30 Dec 2015 00:08:51 +0900, FUJIWARA Katsunori wrote: > # HG changeset patch > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp> > # Date 1451401110 -32400 > # Tue Dec 29 23:58:30 2015 +0900 > # Node ID 0aa9cf12fc6ce052b0f2e91674d1c56fe6078939 > # Parent a1e4c18851155866ab75a35aa5bbae81945d7db5 > revset: use delayregistrar to register predicate in extension easily > > Previous patch introduced 'revset.predicate' decorator to register > revset predicate function easily. > > But it shouldn't be used in extension directly, because it registers > specified function immediately. Registration itself can't be restored, > even if extension loading fails after that. > > Therefore, registration should be delayed until 'uisetup()' or so. > > This patch uses 'extpredicate' decorator derived from 'delayregistrar' > to register predicate in extension easily. > > This patch also tests whether 'registrar.delayregistrar' avoids > function registration if 'setup()' isn't invoked on it, because > 'extpredicate' is the first user of it. > --- a/hgext/mq.py > +++ b/hgext/mq.py > @@ -3558,9 +3558,11 @@ def summaryhook(ui, repo): > # i18n: column positioning for "hg summary" > ui.note(_("mq: (empty queue)\n")) > > +revsetpredicate = revset.extpredicate() > + > +@revsetpredicate('mq()') > def revsetmq(repo, subset, x): > - """``mq()`` > - Changesets managed by MQ. > + """Changesets managed by MQ. > """ > revset.getargs(x, 0, 0, _("mq takes no arguments")) > applied = set([repo[r.node].rev() for r in repo.mq.applied]) > @@ -3596,7 +3598,7 @@ def extsetup(ui): > if extmodule.__file__ != __file__: > dotable(getattr(extmodule, 'cmdtable', {})) > > - revset.symbols['mq'] = revsetmq > + revsetpredicate.setup() "cmdtable" was wrong? I like consistent extension APIs, so my idea was something like this: 1. define table of revset symbols per extension # creates new table and associated decorator revsetpredicate = registrar.revsetpredicate() # or revsetsymbols = {} revsetpredicate = registrar.revsetpredicate(revsetsymbols) # revsetpredicate['mq'] = revsetmq @revsetpredicate('mq()') def revsetmq(...): ... 2. load them by dispatch._dispatch()
At Fri, 1 Jan 2016 17:53:10 +0900, Yuya Nishihara wrote: > > On Wed, 30 Dec 2015 00:08:51 +0900, FUJIWARA Katsunori wrote: > > # HG changeset patch > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp> > > # Date 1451401110 -32400 > > # Tue Dec 29 23:58:30 2015 +0900 > > # Node ID 0aa9cf12fc6ce052b0f2e91674d1c56fe6078939 > > # Parent a1e4c18851155866ab75a35aa5bbae81945d7db5 > > revset: use delayregistrar to register predicate in extension easily > > > > Previous patch introduced 'revset.predicate' decorator to register > > revset predicate function easily. > > > > But it shouldn't be used in extension directly, because it registers > > specified function immediately. Registration itself can't be restored, > > even if extension loading fails after that. > > > > Therefore, registration should be delayed until 'uisetup()' or so. > > > > This patch uses 'extpredicate' decorator derived from 'delayregistrar' > > to register predicate in extension easily. > > > > This patch also tests whether 'registrar.delayregistrar' avoids > > function registration if 'setup()' isn't invoked on it, because > > 'extpredicate' is the first user of it. > > > --- a/hgext/mq.py > > +++ b/hgext/mq.py > > @@ -3558,9 +3558,11 @@ def summaryhook(ui, repo): > > # i18n: column positioning for "hg summary" > > ui.note(_("mq: (empty queue)\n")) > > > > +revsetpredicate = revset.extpredicate() > > + > > +@revsetpredicate('mq()') > > def revsetmq(repo, subset, x): > > - """``mq()`` > > - Changesets managed by MQ. > > + """Changesets managed by MQ. > > """ > > revset.getargs(x, 0, 0, _("mq takes no arguments")) > > applied = set([repo[r.node].rev() for r in repo.mq.applied]) > > @@ -3596,7 +3598,7 @@ def extsetup(ui): > > if extmodule.__file__ != __file__: > > dotable(getattr(extmodule, 'cmdtable', {})) > > > > - revset.symbols['mq'] = revsetmq > > + revsetpredicate.setup() Augie, could you drop this series from clowncopter if it is possible ? It will make re-work easy. > "cmdtable" was wrong? > > I like consistent extension APIs, so my idea was something like this: > > 1. define table of revset symbols per extension > > # creates new table and associated decorator > revsetpredicate = registrar.revsetpredicate() In this case, loading logic looks inside 'revsetpredicate' object to get predicate functions to be loaded, doesn't it ? (= table-ness of bridging object isn't important) > # or > revsetsymbols = {} > revsetpredicate = registrar.revsetpredicate(revsetsymbols) (BTW, would you also mean that revset decorator specific code should be placed in registrar.py by sample code above ? I think that it shouldn't so, IMHO) > # revsetpredicate['mq'] = revsetmq > @revsetpredicate('mq()') > def revsetmq(...): > ... > > 2. load them by dispatch._dispatch() What about registration steps below, if explicit setup() on revsetpredicate isn't suitable for consistent API ? This can prevents dispatch (and registrar) from depending on revset, template XXXX, and so on. dispatch._dispatch() - extensions.loadall() - at loading extension - instantiate revset.predicate or so as 'revsetpredicate' (*1) - at loading revset.py - register revset specific hook (*2) to registrar.py - revset.predicate() - decorator stores information internally - uisetup of extension - extsetup of extension - invoke hooks registered in registrar.py on each extensions - invoke hook (*2) - load predicates from 'revsetpredicate' attribute (*1) of extension ---------------------------------------------------------------------- [FUJIWARA Katsunori] foozy@lares.dti.ne.jp
At Sat, 02 Jan 2016 01:28:28 +0900, FUJIWARA Katsunori wrote: > > At Fri, 1 Jan 2016 17:53:10 +0900, > Yuya Nishihara wrote: > > > > On Wed, 30 Dec 2015 00:08:51 +0900, FUJIWARA Katsunori wrote: > > > # HG changeset patch > > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp> > > > # Date 1451401110 -32400 > > > # Tue Dec 29 23:58:30 2015 +0900 > > > # Node ID 0aa9cf12fc6ce052b0f2e91674d1c56fe6078939 > > > # Parent a1e4c18851155866ab75a35aa5bbae81945d7db5 > > > revset: use delayregistrar to register predicate in extension easily > > > > > > Previous patch introduced 'revset.predicate' decorator to register > > > revset predicate function easily. > > > > > > But it shouldn't be used in extension directly, because it registers > > > specified function immediately. Registration itself can't be restored, > > > even if extension loading fails after that. > > > > > > Therefore, registration should be delayed until 'uisetup()' or so. > > > > > > This patch uses 'extpredicate' decorator derived from 'delayregistrar' > > > to register predicate in extension easily. > > > > > > This patch also tests whether 'registrar.delayregistrar' avoids > > > function registration if 'setup()' isn't invoked on it, because > > > 'extpredicate' is the first user of it. > > > > > --- a/hgext/mq.py > > > +++ b/hgext/mq.py > > > @@ -3558,9 +3558,11 @@ def summaryhook(ui, repo): > > > # i18n: column positioning for "hg summary" > > > ui.note(_("mq: (empty queue)\n")) > > > > > > +revsetpredicate = revset.extpredicate() > > > + > > > +@revsetpredicate('mq()') > > > def revsetmq(repo, subset, x): > > > - """``mq()`` > > > - Changesets managed by MQ. > > > + """Changesets managed by MQ. > > > """ > > > revset.getargs(x, 0, 0, _("mq takes no arguments")) > > > applied = set([repo[r.node].rev() for r in repo.mq.applied]) > > > @@ -3596,7 +3598,7 @@ def extsetup(ui): > > > if extmodule.__file__ != __file__: > > > dotable(getattr(extmodule, 'cmdtable', {})) > > > > > > - revset.symbols['mq'] = revsetmq > > > + revsetpredicate.setup() > > Augie, could you drop this series from clowncopter if it is possible ? > It will make re-work easy. Oh, this series was already imported into master repo. I'll make changes on recent tip, if needed :-) ---------------------------------------------------------------------- [FUJIWARA Katsunori] foozy@lares.dti.ne.jp
On Sat, 02 Jan 2016 01:28:28 +0900, FUJIWARA Katsunori wrote: > At Fri, 1 Jan 2016 17:53:10 +0900, > Yuya Nishihara wrote: > > "cmdtable" was wrong? > > > > I like consistent extension APIs, so my idea was something like this: > > > > 1. define table of revset symbols per extension > > > > # creates new table and associated decorator > > revsetpredicate = registrar.revsetpredicate() > > In this case, loading logic looks inside 'revsetpredicate' object to > get predicate functions to be loaded, doesn't it ? (= table-ness of > bridging object isn't important) Right. The difference is who controls the loading logic. (a) @command mercurial.dispatch loads cmdtable (b) @revsetpredicate hgext.xxx updates revset.symbols We haven't (a) for revsets and templates, and your patch follows it, which is straightforward. I just wondered if (a) is preferable as an extension API. > > revsetpredicate = registrar.revsetpredicate(revsetsymbols) > > (BTW, would you also mean that revset decorator specific code should > be placed in registrar.py by sample code above ? I think that it > shouldn't so, IMHO) Yes. If the registrar provides all decorators, an extension can avoid importing templatekw or templatefilters just because it defines new template keywords/filters. (though the revset module would be necessary to define new workable revset predicates.) > > 2. load them by dispatch._dispatch() > > What about registration steps below, if explicit setup() on > revsetpredicate isn't suitable for consistent API ? This can prevents > dispatch (and registrar) from depending on revset, template XXXX, and > so on. > > dispatch._dispatch() > - extensions.loadall() > - at loading extension > - instantiate revset.predicate or so as 'revsetpredicate' (*1) > - at loading revset.py > - register revset specific hook (*2) to registrar.py > - revset.predicate() > - decorator stores information internally > - uisetup of extension > - extsetup of extension > - invoke hooks registered in registrar.py on each extensions > - invoke hook (*2) > - load predicates from 'revsetpredicate' attribute (*1) of extension registrar shouldn't import revset, but I think it's fine for dispatch to import revset. And import-time hook registration won't be necessary. Regards,
At Sat, 2 Jan 2016 17:56:59 +0900, Yuya Nishihara wrote: > > On Sat, 02 Jan 2016 01:28:28 +0900, FUJIWARA Katsunori wrote: > > At Fri, 1 Jan 2016 17:53:10 +0900, > > Yuya Nishihara wrote: > > > "cmdtable" was wrong? > > > > > > I like consistent extension APIs, so my idea was something like this: > > > > > > 1. define table of revset symbols per extension > > > > > > # creates new table and associated decorator > > > revsetpredicate = registrar.revsetpredicate() > > > > In this case, loading logic looks inside 'revsetpredicate' object to > > get predicate functions to be loaded, doesn't it ? (= table-ness of > > bridging object isn't important) > > Right. The difference is who controls the loading logic. > > (a) @command mercurial.dispatch loads cmdtable > (b) @revsetpredicate hgext.xxx updates revset.symbols > > We haven't (a) for revsets and templates, and your patch follows it, > which is straightforward. I just wondered if (a) is preferable as an extension > API. Thank you for explanation. > > > revsetpredicate = registrar.revsetpredicate(revsetsymbols) > > > > (BTW, would you also mean that revset decorator specific code should > > be placed in registrar.py by sample code above ? I think that it > > shouldn't so, IMHO) > > Yes. If the registrar provides all decorators, an extension can avoid > importing templatekw or templatefilters just because it defines new template > keywords/filters. (though the revset module would be necessary to define new > workable revset predicates.) Yeah, I though that placing decorator in each corresponded module is reasonable, because defining new workable function requires importing corresponded module in many cases, to use utilities in it: revset, fileset, some template keyword listing elements or so, for example. But it is obvious that module importing should be avoided if possible, as you described. > > > 2. load them by dispatch._dispatch() > > > > What about registration steps below, if explicit setup() on > > revsetpredicate isn't suitable for consistent API ? This can prevents > > dispatch (and registrar) from depending on revset, template XXXX, and > > so on. > > > > dispatch._dispatch() > > - extensions.loadall() > > - at loading extension > > - instantiate revset.predicate or so as 'revsetpredicate' (*1) > > - at loading revset.py > > - register revset specific hook (*2) to registrar.py > > - revset.predicate() > > - decorator stores information internally > > - uisetup of extension > > - extsetup of extension > > - invoke hooks registered in registrar.py on each extensions > > - invoke hook (*2) > > - load predicates from 'revsetpredicate' attribute (*1) of extension > > registrar shouldn't import revset, but I think it's fine for dispatch to > import revset. And import-time hook registration won't be necessary. OK, I'll send another series to revise this series in such manner. > Regards, > ---------------------------------------------------------------------- [FUJIWARA Katsunori] foozy@lares.dti.ne.jp
Patch
diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py --- a/hgext/largefiles/overrides.py +++ b/hgext/largefiles/overrides.py @@ -812,9 +812,11 @@ def overridepull(orig, ui, repo, source= ui.status(_("%d largefiles cached\n") % numcached) return result +revsetpredicate = revset.extpredicate() + +@revsetpredicate('pulled()') def pulledrevsetsymbol(repo, subset, x): - """``pulled()`` - Changesets that just has been pulled. + """Changesets that just has been pulled. Only available with largefiles from pull --lfrev expressions. diff --git a/hgext/largefiles/uisetup.py b/hgext/largefiles/uisetup.py --- a/hgext/largefiles/uisetup.py +++ b/hgext/largefiles/uisetup.py @@ -9,7 +9,7 @@ '''setup for largefiles extension: uisetup''' from mercurial import archival, cmdutil, commands, extensions, filemerge, hg, \ - httppeer, merge, scmutil, sshpeer, wireproto, revset, subrepo, copies + httppeer, merge, scmutil, sshpeer, wireproto, subrepo, copies from mercurial.i18n import _ from mercurial.hgweb import hgweb_mod, webcommands @@ -83,7 +83,6 @@ def uisetup(ui): ('', 'lfrev', [], _('download largefiles for these revisions'), _('REV'))] entry[1].extend(pullopt) - revset.symbols['pulled'] = overrides.pulledrevsetsymbol entry = extensions.wrapcommand(commands.table, 'clone', overrides.overrideclone) @@ -170,3 +169,5 @@ def uisetup(ui): if name == 'transplant': extensions.wrapcommand(getattr(module, 'cmdtable'), 'transplant', overrides.overridetransplant) + + overrides.revsetpredicate.setup() diff --git a/hgext/mq.py b/hgext/mq.py --- a/hgext/mq.py +++ b/hgext/mq.py @@ -3558,9 +3558,11 @@ def summaryhook(ui, repo): # i18n: column positioning for "hg summary" ui.note(_("mq: (empty queue)\n")) +revsetpredicate = revset.extpredicate() + +@revsetpredicate('mq()') def revsetmq(repo, subset, x): - """``mq()`` - Changesets managed by MQ. + """Changesets managed by MQ. """ revset.getargs(x, 0, 0, _("mq takes no arguments")) applied = set([repo[r.node].rev() for r in repo.mq.applied]) @@ -3596,7 +3598,7 @@ def extsetup(ui): if extmodule.__file__ != __file__: dotable(getattr(extmodule, 'cmdtable', {})) - revset.symbols['mq'] = revsetmq + revsetpredicate.setup() colortable = {'qguard.negative': 'red', 'qguard.positive': 'yellow', diff --git a/hgext/rebase.py b/hgext/rebase.py --- a/hgext/rebase.py +++ b/hgext/rebase.py @@ -64,6 +64,9 @@ def _destrebase(repo): branch = repo[None].branch() return repo[branch].rev() +revsetpredicate = revset.extpredicate() + +@revsetpredicate('_destrebase') def _revsetdestrebase(repo, subset, x): # ``_rebasedefaultdest()`` @@ -1231,4 +1234,4 @@ def uisetup(ui): _("use 'hg rebase --continue' or 'hg rebase --abort'")]) # ensure rebased rev are not hidden extensions.wrapfunction(repoview, '_getdynamicblockers', _rebasedvisible) - revset.symbols['_destrebase'] = _revsetdestrebase + revsetpredicate.setup() diff --git a/hgext/transplant.py b/hgext/transplant.py --- a/hgext/transplant.py +++ b/hgext/transplant.py @@ -693,9 +693,11 @@ def _dotransplant(ui, repo, *revs, **opt if cleanupfn: cleanupfn() +revsetpredicate = revset.extpredicate() + +@revsetpredicate('transplanted([set])') def revsettransplanted(repo, subset, x): - """``transplanted([set])`` - Transplanted changesets in set, or all transplanted changesets. + """Transplanted changesets in set, or all transplanted changesets. """ if x: s = revset.getset(repo, subset, x) @@ -711,7 +713,7 @@ def kwtransplanted(repo, ctx, **args): return n and revlog.hex(n) or '' def extsetup(ui): - revset.symbols['transplanted'] = revsettransplanted + revsetpredicate.setup() templatekw.keywords['transplanted'] = kwtransplanted cmdutil.unfinishedstates.append( ['series', True, False, _('transplant in progress'), diff --git a/mercurial/revset.py b/mercurial/revset.py --- a/mercurial/revset.py +++ b/mercurial/revset.py @@ -487,11 +487,36 @@ class predicate(registrar.funcregistrar) The first string argument of the constructor is used also in online help. + + Use 'extpredicate' instead of this to register revset predicate in + extensions. """ table = symbols formatdoc = "``%s``\n %s" getname = registrar.funcregistrar.parsefuncdecl +class extpredicate(registrar.delayregistrar): + """Decorator to register revset predicate in extensions + + Usage:: + + revsetpredicate = revset.extpredicate() + + @revsetpredicate('mypredicate(arg1, arg2[, arg3])') + def mypredicatefunc(repo, subset, x): + '''Explanation of this revset predicate .... + ''' + pass + + def uisetup(ui): + revsetpredicate.setup() + + 'revsetpredicate' instance above can be used to decorate multiple + functions, and 'setup()' on it registers all such functions at + once. + """ + registrar = predicate + @predicate('_destupdate') def _destupdate(repo, subset, x): # experimental revset for update destination diff --git a/tests/test-revset.t b/tests/test-revset.t --- a/tests/test-revset.t +++ b/tests/test-revset.t @@ -2189,3 +2189,43 @@ test error message of bad revset [255] $ cd .. + +Test registrar.delayregistrar via revset.extpredicate + +'extpredicate' decorator shouldn't register any functions until +'setup()' on it. + + $ cd repo + + $ cat <<EOF > $TESTTMP/custompredicate.py + > from mercurial import revset + > + > revsetpredicate = revset.extpredicate() + > + > @revsetpredicate('custom1()') + > def custom1(repo, subset, x): + > return revset.baseset([1]) + > @revsetpredicate('custom2()') + > def custom2(repo, subset, x): + > return revset.baseset([2]) + > + > def uisetup(ui): + > if ui.configbool('custompredicate', 'enabled'): + > revsetpredicate.setup() + > EOF + $ cat <<EOF > .hg/hgrc + > [extensions] + > custompredicate = $TESTTMP/custompredicate.py + > EOF + + $ hg debugrevspec "custom1()" + hg: parse error: unknown identifier: custom1 + [255] + $ hg debugrevspec "custom2()" + hg: parse error: unknown identifier: custom2 + [255] + $ hg debugrevspec "custom1() or custom2()" --config custompredicate.enabled=true + 1 + 2 + + $ cd ..