Patchwork [3,of,3] histedit: replace @addhisteditaction with @action

login
register
mail settings
Submitter timeless@mozdev.org
Date Jan. 5, 2016, 6:31 p.m.
Message ID <5b616db6e64040d13815.1452018713@waste.org>
Download mbox | patch
Permalink /patch/12540/
State Accepted
Headers show

Comments

timeless@mozdev.org - Jan. 5, 2016, 6:31 p.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1450906238 0
#      Wed Dec 23 21:30:38 2015 +0000
# Node ID 5b616db6e64040d138152297bff3b62245167917
# Parent  0e22d25a49546b65a4eb53ef53f555634afdbef9
histedit: replace @addhisteditaction with @action

@action supports verbs, messages, priority, and internal

messages should be translated.
internal means the action should not be listed.

geteditcomment will construct the verbs list based on
@actions (prefering priority over non priority, otherwise
favoring verbs with short forms over verbs without).
Mateusz Kwapich - Jan. 5, 2016, 9:24 p.m.
I’m back from my christmas vacation :) I’m fine with this change - it’s even better than my previous change
Which was supposed to be queued but somehow was skipped  ( http://patchwork.serpentine.com/patch/11938/ )

My goal with histedit architectural changes was to make sure that adding a new command doesn’t require any
soccery. This patch fits nicely into that plan so it looks good to me. 

Best,
Mateusz




On 1/5/16, 10:31 AM, "Mercurial-devel on behalf of timeless" <mercurial-devel-bounces@selenic.com on behalf of timeless@mozdev.org> wrote:

># HG changeset patch

># User timeless <timeless@mozdev.org>

># Date 1450906238 0

>#      Wed Dec 23 21:30:38 2015 +0000

># Node ID 5b616db6e64040d138152297bff3b62245167917

># Parent  0e22d25a49546b65a4eb53ef53f555634afdbef9

>histedit: replace @addhisteditaction with @action

>

>@action supports verbs, messages, priority, and internal

>

>messages should be translated.

>internal means the action should not be listed.

>

>geteditcomment will construct the verbs list based on

>@actions (prefering priority over non priority, otherwise

>favoring verbs with short forms over verbs without).

>

>diff --git a/hgext/histedit.py b/hgext/histedit.py

>--- a/hgext/histedit.py

>+++ b/hgext/histedit.py

>@@ -214,12 +214,19 @@

> # leave the attribute unspecified.

> testedwith = 'internal'

> 

>+actiontable = {}

>+primaryactions = set()

>+secondaryactions = set()

>+tertiaryactions = set()

>+internalactions = set()

>+

> def geteditcomment(first, last):

>     """ construct the editor comment

>     The comment includes::

>      - an intro

>      - sorted primary commands

>      - sorted short commands

>+     - sorted long commands

> 

>     Commands are only included once.

>     """

>@@ -227,19 +234,27 @@

> 

> Commits are listed from least to most recent

> 

>-Commands:""")

>-    # i18n: command names and abbreviations must remain untranslated

>-    verbs = _("""

>- e, edit = use commit, but stop for amending

>- m, mess = edit commit message without changing commit content

>- p, pick = use commit

>- d, drop = remove commit from history

>- f, fold = use commit, but combine it with the one above

>- r, roll = like fold, but discard this commit's description

>+Commands:

> """)

>+    actions = []

>+    def addverb(v):

>+        a = actiontable[v]

>+        lines = a.message.split("\n")

>+        if len(a.verbs):

>+            v = ', '.join(sorted(a.verbs, key=lambda v: len(v)))

>+        actions.append(" %s = %s" % (v, lines[0]))

>+        actions.extend(['  %s' for l in lines[1:]])

>+

>+    for v in (

>+         sorted(primaryactions) +

>+         sorted(secondaryactions) +

>+         sorted(tertiaryactions)

>+        ):

>+        addverb(v)

>+    actions.append('')

> 

>     return ''.join(['# %s\n' % l if l else '#\n'

>-                    for l in ((intro % (first, last) + verbs).split('\n'))])

>+                    for l in ((intro % (first, last)).split('\n')) + actions])

> 

> class histeditstate(object):

>     def __init__(self, repo, parentctxnode=None, actions=None, keep=None,

>@@ -596,21 +611,30 @@

>         hint=_('amend, commit, or revert them and run histedit '

>             '--continue, or abort with histedit --abort'))

> 

>+def action(verbs, message, priority=False, internal=False):

>+    def wrap(cls):

>+        assert not priority or not internal

>+        verb = verbs[0]

>+        if priority:

>+            primaryactions.add(verb)

>+        elif internal:

>+            internalactions.add(verb)

>+        elif len(verbs) > 1:

>+            secondaryactions.add(verb)

>+        else:

>+            tertiaryactions.add(verb)

> 

>-actiontable = {}

>-actionlist = []

>-

>-def addhisteditaction(verbs):

>-    def wrap(cls):

>-        cls.verb = verbs[0]

>+        cls.verb = verb

>+        cls.verbs = verbs

>+        cls.message = message

>         for verb in verbs:

>             actiontable[verb] = cls

>-        actionlist.append(cls)

>         return cls

>     return wrap

> 

>-

>-@addhisteditaction(['pick', 'p'])

>+@action(['pick', 'p'],

>+        _('use commit'),

>+        priority=True)

> class pick(histeditaction):

>     def run(self):

>         rulectx = self.repo[self.node]

>@@ -620,7 +644,9 @@

> 

>         return super(pick, self).run()

> 

>-@addhisteditaction(['edit', 'e'])

>+@action(['edit', 'e'],

>+        _('use commit, but stop for amending'),

>+        priority=True)

> class edit(histeditaction):

>     def run(self):

>         repo = self.repo

>@@ -635,7 +661,8 @@

>     def commiteditor(self):

>         return cmdutil.getcommiteditor(edit=True, editform='histedit.edit')

> 

>-@addhisteditaction(['fold', 'f'])

>+@action(['fold', 'f'],

>+        _('use commit, but combine it with the one above'))

> class fold(histeditaction):

>     def verify(self, prev):

>         """ Verifies semantic correctness of the fold rule"""

>@@ -761,8 +788,8 @@

>         basectx = self.repo['.']

>         return basectx, []

> 

>-@addhisteditaction(['_multifold'])

>-class _multifold(fold):

>+@action(['_multifold'],

>+        _(

>     """fold subclass used for when multiple folds happen in a row

> 

>     We only want to fire the editor for the folded message once when

>@@ -770,11 +797,14 @@

>     similar to rollup, but we should preserve both messages so that

>     when the last fold operation runs we can show the user all the

>     commit messages in their editor.

>-    """

>+    """),

>+        internal=True)

>+class _multifold(fold):

>     def skipprompt(self):

>         return True

> 

>-@addhisteditaction(["roll", "r"])

>+@action(["roll", "r"],

>+        _("like fold, but discard this commit's description"))

> class rollup(fold):

>     def mergedescs(self):

>         return False

>@@ -782,13 +812,16 @@

>     def skipprompt(self):

>         return True

> 

>-@addhisteditaction(["drop", "d"])

>+@action(["drop", "d"],

>+        _('remove commit from history'))

> class drop(histeditaction):

>     def run(self):

>         parentctx = self.repo[self.state.parentctxnode]

>         return parentctx, [(self.node, tuple())]

> 

>-@addhisteditaction(["mess", "m"])

>+@action(["mess", "m"],

>+        _('edit commit message without changing commit content'),

>+        priority=True)

> class message(histeditaction):

>     def commiteditor(self):

>         return cmdutil.getcommiteditor(edit=True, editform='histedit.mess')

>@@ -1470,4 +1503,6 @@

>         ['histedit-state', False, True, _('histedit in progress'),

>          _("use 'hg histedit --continue' or 'hg histedit --abort'")])

>     if ui.configbool("experimental", "histeditng"):

>-        globals()['base'] = addhisteditaction(['base', 'b'])(base)

>+        globals()['base'] = action(['base', 'b'],

>+            _('checkout changeset and apply further changesets from there')

>+        )(base)

>_______________________________________________

>Mercurial-devel mailing list

>Mercurial-devel@selenic.com

>https://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Jan. 7, 2016, 12:51 a.m.
On Tue, Jan 05, 2016 at 12:31:53PM -0600, timeless wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1450906238 0
> #      Wed Dec 23 21:30:38 2015 +0000
> # Node ID 5b616db6e64040d138152297bff3b62245167917
> # Parent  0e22d25a49546b65a4eb53ef53f555634afdbef9
> histedit: replace @addhisteditaction with @action

These are queued, thanks.

>
> @action supports verbs, messages, priority, and internal
>
> messages should be translated.
> internal means the action should not be listed.
>
> geteditcomment will construct the verbs list based on
> @actions (prefering priority over non priority, otherwise
> favoring verbs with short forms over verbs without).
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -214,12 +214,19 @@
>  # leave the attribute unspecified.
>  testedwith = 'internal'
>
> +actiontable = {}
> +primaryactions = set()
> +secondaryactions = set()
> +tertiaryactions = set()
> +internalactions = set()
> +
>  def geteditcomment(first, last):
>      """ construct the editor comment
>      The comment includes::
>       - an intro
>       - sorted primary commands
>       - sorted short commands
> +     - sorted long commands
>
>      Commands are only included once.
>      """
> @@ -227,19 +234,27 @@
>
>  Commits are listed from least to most recent
>
> -Commands:""")
> -    # i18n: command names and abbreviations must remain untranslated
> -    verbs = _("""
> - e, edit = use commit, but stop for amending
> - m, mess = edit commit message without changing commit content
> - p, pick = use commit
> - d, drop = remove commit from history
> - f, fold = use commit, but combine it with the one above
> - r, roll = like fold, but discard this commit's description
> +Commands:
>  """)
> +    actions = []
> +    def addverb(v):
> +        a = actiontable[v]
> +        lines = a.message.split("\n")
> +        if len(a.verbs):
> +            v = ', '.join(sorted(a.verbs, key=lambda v: len(v)))
> +        actions.append(" %s = %s" % (v, lines[0]))
> +        actions.extend(['  %s' for l in lines[1:]])
> +
> +    for v in (
> +         sorted(primaryactions) +
> +         sorted(secondaryactions) +
> +         sorted(tertiaryactions)
> +        ):
> +        addverb(v)
> +    actions.append('')
>
>      return ''.join(['# %s\n' % l if l else '#\n'
> -                    for l in ((intro % (first, last) + verbs).split('\n'))])
> +                    for l in ((intro % (first, last)).split('\n')) + actions])
>
>  class histeditstate(object):
>      def __init__(self, repo, parentctxnode=None, actions=None, keep=None,
> @@ -596,21 +611,30 @@
>          hint=_('amend, commit, or revert them and run histedit '
>              '--continue, or abort with histedit --abort'))
>
> +def action(verbs, message, priority=False, internal=False):
> +    def wrap(cls):
> +        assert not priority or not internal
> +        verb = verbs[0]
> +        if priority:
> +            primaryactions.add(verb)
> +        elif internal:
> +            internalactions.add(verb)
> +        elif len(verbs) > 1:
> +            secondaryactions.add(verb)
> +        else:
> +            tertiaryactions.add(verb)
>
> -actiontable = {}
> -actionlist = []
> -
> -def addhisteditaction(verbs):
> -    def wrap(cls):
> -        cls.verb = verbs[0]
> +        cls.verb = verb
> +        cls.verbs = verbs
> +        cls.message = message
>          for verb in verbs:
>              actiontable[verb] = cls
> -        actionlist.append(cls)
>          return cls
>      return wrap
>
> -
> -@addhisteditaction(['pick', 'p'])
> +@action(['pick', 'p'],
> +        _('use commit'),
> +        priority=True)
>  class pick(histeditaction):
>      def run(self):
>          rulectx = self.repo[self.node]
> @@ -620,7 +644,9 @@
>
>          return super(pick, self).run()
>
> -@addhisteditaction(['edit', 'e'])
> +@action(['edit', 'e'],
> +        _('use commit, but stop for amending'),
> +        priority=True)
>  class edit(histeditaction):
>      def run(self):
>          repo = self.repo
> @@ -635,7 +661,8 @@
>      def commiteditor(self):
>          return cmdutil.getcommiteditor(edit=True, editform='histedit.edit')
>
> -@addhisteditaction(['fold', 'f'])
> +@action(['fold', 'f'],
> +        _('use commit, but combine it with the one above'))
>  class fold(histeditaction):
>      def verify(self, prev):
>          """ Verifies semantic correctness of the fold rule"""
> @@ -761,8 +788,8 @@
>          basectx = self.repo['.']
>          return basectx, []
>
> -@addhisteditaction(['_multifold'])
> -class _multifold(fold):
> +@action(['_multifold'],
> +        _(
>      """fold subclass used for when multiple folds happen in a row
>
>      We only want to fire the editor for the folded message once when
> @@ -770,11 +797,14 @@
>      similar to rollup, but we should preserve both messages so that
>      when the last fold operation runs we can show the user all the
>      commit messages in their editor.
> -    """
> +    """),
> +        internal=True)
> +class _multifold(fold):
>      def skipprompt(self):
>          return True
>
> -@addhisteditaction(["roll", "r"])
> +@action(["roll", "r"],
> +        _("like fold, but discard this commit's description"))
>  class rollup(fold):
>      def mergedescs(self):
>          return False
> @@ -782,13 +812,16 @@
>      def skipprompt(self):
>          return True
>
> -@addhisteditaction(["drop", "d"])
> +@action(["drop", "d"],
> +        _('remove commit from history'))
>  class drop(histeditaction):
>      def run(self):
>          parentctx = self.repo[self.state.parentctxnode]
>          return parentctx, [(self.node, tuple())]
>
> -@addhisteditaction(["mess", "m"])
> +@action(["mess", "m"],
> +        _('edit commit message without changing commit content'),
> +        priority=True)
>  class message(histeditaction):
>      def commiteditor(self):
>          return cmdutil.getcommiteditor(edit=True, editform='histedit.mess')
> @@ -1470,4 +1503,6 @@
>          ['histedit-state', False, True, _('histedit in progress'),
>           _("use 'hg histedit --continue' or 'hg histedit --abort'")])
>      if ui.configbool("experimental", "histeditng"):
> -        globals()['base'] = addhisteditaction(['base', 'b'])(base)
> +        globals()['base'] = action(['base', 'b'],
> +            _('checkout changeset and apply further changesets from there')
> +        )(base)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Durham Goode - Jan. 20, 2016, 2:50 a.m.
On 1/5/16 10:31 AM, timeless wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1450906238 0
> #      Wed Dec 23 21:30:38 2015 +0000
> # Node ID 5b616db6e64040d138152297bff3b62245167917
> # Parent  0e22d25a49546b65a4eb53ef53f555634afdbef9
> histedit: replace @addhisteditaction with @action
>
> @action supports verbs, messages, priority, and internal
>
> messages should be translated.
> internal means the action should not be listed.
>
> geteditcomment will construct the verbs list based on
> @actions (prefering priority over non priority, otherwise
> favoring verbs with short forms over verbs without).
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -214,12 +214,19 @@
>   # leave the attribute unspecified.
>   testedwith = 'internal'
>   
> +actiontable = {}
> +primaryactions = set()
> +secondaryactions = set()
> +tertiaryactions = set()
> +internalactions = set()
> +
I just stumbled on this code and was confused.  What is the difference 
between these 4?  Is it just for sorting in the help text? This seems 
like a strange way of representing sort order.  Why not give each a 
priority value (from an enum) and just sort on it?
Augie Fackler - Jan. 20, 2016, 3:51 a.m.
> On Jan 19, 2016, at 9:50 PM, Durham Goode <durham@fb.com> wrote:
> 
> 
> 
> On 1/5/16 10:31 AM, timeless wrote:
>> # HG changeset patch
>> # User timeless <timeless@mozdev.org>
>> # Date 1450906238 0
>> #      Wed Dec 23 21:30:38 2015 +0000
>> # Node ID 5b616db6e64040d138152297bff3b62245167917
>> # Parent  0e22d25a49546b65a4eb53ef53f555634afdbef9
>> histedit: replace @addhisteditaction with @action
>> 
>> @action supports verbs, messages, priority, and internal
>> 
>> messages should be translated.
>> internal means the action should not be listed.
>> 
>> geteditcomment will construct the verbs list based on
>> @actions (prefering priority over non priority, otherwise
>> favoring verbs with short forms over verbs without).
>> 
>> diff --git a/hgext/histedit.py b/hgext/histedit.py
>> --- a/hgext/histedit.py
>> +++ b/hgext/histedit.py
>> @@ -214,12 +214,19 @@
>>  # leave the attribute unspecified.
>>  testedwith = 'internal'
>>  +actiontable = {}
>> +primaryactions = set()
>> +secondaryactions = set()
>> +tertiaryactions = set()
>> +internalactions = set()
>> +
> I just stumbled on this code and was confused.  What is the difference between these 4?  Is it just for sorting in the help text?

Yep. Just for ordering so the “most valuable” ones are at the top.

> This seems like a strange way of representing sort order.  Why not give each a priority value (from an enum) and just sort on it?

I don’t really feel strongly. If you do, I encourage and welcome follow up patches. ;)

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
timeless - Jan. 20, 2016, 5:15 p.m.
On Tue, Jan 19, 2016 at 9:50 PM, Durham Goode <durham@fb.com> wrote:
>> +primaryactions = set()
>> +secondaryactions = set()
>> +tertiaryactions = set()
>> +internalactions = set()
>
> I just stumbled on this code and was confused.  What is the difference
> between these 4?

primary are special actions which we think are the most important
(should be first).
internal are things that histedit itself uses for state transitions
and which shouldn't be selected by users.
tertiary are things which don't have a short code and are thus much
less important.
secondary are things with a short code, but which aren't the most important.

> Is it just for sorting in the help text?

Yes, it's just sorting the help. Internal don't actually appear in the help.

> This seems like a strange way of representing sort order.
> Why not give each a priority value (from an enum) and just sort on it?

I don't really want people creating arbitrary enum values, e.g. 15.
I also don't want third party extensions to select things as "primary".
Your addins should fall naturally into internal/tertiary/secondary.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -214,12 +214,19 @@ 
 # leave the attribute unspecified.
 testedwith = 'internal'
 
+actiontable = {}
+primaryactions = set()
+secondaryactions = set()
+tertiaryactions = set()
+internalactions = set()
+
 def geteditcomment(first, last):
     """ construct the editor comment
     The comment includes::
      - an intro
      - sorted primary commands
      - sorted short commands
+     - sorted long commands
 
     Commands are only included once.
     """
@@ -227,19 +234,27 @@ 
 
 Commits are listed from least to most recent
 
-Commands:""")
-    # i18n: command names and abbreviations must remain untranslated
-    verbs = _("""
- e, edit = use commit, but stop for amending
- m, mess = edit commit message without changing commit content
- p, pick = use commit
- d, drop = remove commit from history
- f, fold = use commit, but combine it with the one above
- r, roll = like fold, but discard this commit's description
+Commands:
 """)
+    actions = []
+    def addverb(v):
+        a = actiontable[v]
+        lines = a.message.split("\n")
+        if len(a.verbs):
+            v = ', '.join(sorted(a.verbs, key=lambda v: len(v)))
+        actions.append(" %s = %s" % (v, lines[0]))
+        actions.extend(['  %s' for l in lines[1:]])
+
+    for v in (
+         sorted(primaryactions) +
+         sorted(secondaryactions) +
+         sorted(tertiaryactions)
+        ):
+        addverb(v)
+    actions.append('')
 
     return ''.join(['# %s\n' % l if l else '#\n'
-                    for l in ((intro % (first, last) + verbs).split('\n'))])
+                    for l in ((intro % (first, last)).split('\n')) + actions])
 
 class histeditstate(object):
     def __init__(self, repo, parentctxnode=None, actions=None, keep=None,
@@ -596,21 +611,30 @@ 
         hint=_('amend, commit, or revert them and run histedit '
             '--continue, or abort with histedit --abort'))
 
+def action(verbs, message, priority=False, internal=False):
+    def wrap(cls):
+        assert not priority or not internal
+        verb = verbs[0]
+        if priority:
+            primaryactions.add(verb)
+        elif internal:
+            internalactions.add(verb)
+        elif len(verbs) > 1:
+            secondaryactions.add(verb)
+        else:
+            tertiaryactions.add(verb)
 
-actiontable = {}
-actionlist = []
-
-def addhisteditaction(verbs):
-    def wrap(cls):
-        cls.verb = verbs[0]
+        cls.verb = verb
+        cls.verbs = verbs
+        cls.message = message
         for verb in verbs:
             actiontable[verb] = cls
-        actionlist.append(cls)
         return cls
     return wrap
 
-
-@addhisteditaction(['pick', 'p'])
+@action(['pick', 'p'],
+        _('use commit'),
+        priority=True)
 class pick(histeditaction):
     def run(self):
         rulectx = self.repo[self.node]
@@ -620,7 +644,9 @@ 
 
         return super(pick, self).run()
 
-@addhisteditaction(['edit', 'e'])
+@action(['edit', 'e'],
+        _('use commit, but stop for amending'),
+        priority=True)
 class edit(histeditaction):
     def run(self):
         repo = self.repo
@@ -635,7 +661,8 @@ 
     def commiteditor(self):
         return cmdutil.getcommiteditor(edit=True, editform='histedit.edit')
 
-@addhisteditaction(['fold', 'f'])
+@action(['fold', 'f'],
+        _('use commit, but combine it with the one above'))
 class fold(histeditaction):
     def verify(self, prev):
         """ Verifies semantic correctness of the fold rule"""
@@ -761,8 +788,8 @@ 
         basectx = self.repo['.']
         return basectx, []
 
-@addhisteditaction(['_multifold'])
-class _multifold(fold):
+@action(['_multifold'],
+        _(
     """fold subclass used for when multiple folds happen in a row
 
     We only want to fire the editor for the folded message once when
@@ -770,11 +797,14 @@ 
     similar to rollup, but we should preserve both messages so that
     when the last fold operation runs we can show the user all the
     commit messages in their editor.
-    """
+    """),
+        internal=True)
+class _multifold(fold):
     def skipprompt(self):
         return True
 
-@addhisteditaction(["roll", "r"])
+@action(["roll", "r"],
+        _("like fold, but discard this commit's description"))
 class rollup(fold):
     def mergedescs(self):
         return False
@@ -782,13 +812,16 @@ 
     def skipprompt(self):
         return True
 
-@addhisteditaction(["drop", "d"])
+@action(["drop", "d"],
+        _('remove commit from history'))
 class drop(histeditaction):
     def run(self):
         parentctx = self.repo[self.state.parentctxnode]
         return parentctx, [(self.node, tuple())]
 
-@addhisteditaction(["mess", "m"])
+@action(["mess", "m"],
+        _('edit commit message without changing commit content'),
+        priority=True)
 class message(histeditaction):
     def commiteditor(self):
         return cmdutil.getcommiteditor(edit=True, editform='histedit.mess')
@@ -1470,4 +1503,6 @@ 
         ['histedit-state', False, True, _('histedit in progress'),
          _("use 'hg histedit --continue' or 'hg histedit --abort'")])
     if ui.configbool("experimental", "histeditng"):
-        globals()['base'] = addhisteditaction(['base', 'b'])(base)
+        globals()['base'] = action(['base', 'b'],
+            _('checkout changeset and apply further changesets from there')
+        )(base)