Patchwork [1,of,6] histedit: move autoverb logic from torule to ruleeditor

login
register
mail settings
Submitter Sean Farley
Date June 21, 2016, 11:32 p.m.
Message ID <01212d1f93a2a6c5c736.1466551961@laptop.local>
Download mbox | patch
Permalink /patch/15567/
State Changes Requested
Headers show

Comments

Sean Farley - June 21, 2016, 11:32 p.m.
# HG changeset patch
# User Sean Farley <sean@farley.io>
# Date 1464306370 25200
#      Thu May 26 16:46:10 2016 -0700
# Node ID 01212d1f93a2a6c5c736dc0c7d45ffd1930e5c2f
# Parent  aa1d56003872cba207d908706da059141dd901a5
# EXP-Topic autoverb
histedit: move autoverb logic from torule to ruleeditor

This is needed for an upcoming change that will automatically rearrange the
rules based on the commit message.
Pierre-Yves David - June 30, 2016, 1:47 a.m.
On 06/22/2016 01:32 AM, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean@farley.io>
> # Date 1464306370 25200
> #      Thu May 26 16:46:10 2016 -0700
> # Node ID 01212d1f93a2a6c5c736dc0c7d45ffd1930e5c2f
> # Parent  aa1d56003872cba207d908706da059141dd901a5
> # EXP-Topic autoverb
> histedit: move autoverb logic from torule to ruleeditor

This series is taking me more time to review than it could. It mostly
come from the fact that the commit description are not very helpful to me.

The descriptions in this series are extremely factual and descriptive
about the patch and does not contains much context. In practice, this
mean than the description mostly duplicate the patch information without
helping me to understand why the change is the right thing to do.

For example, on this first changeset, you are moving logic X from class
A to class B. It would help me to know what is the semantic of class A
and B and why X fit better in B. Otherwise reviewing this series
requires me to re-analyze the whole source of histedit in order to
understand what is going on here. If you could write down the principle
you used decide how to move forward, it would allow me (and any other
reader) to first quickly validate the principle and then validate the
change based on that principle. Because of this, that series have been
delayed in my "second pass in more depth bucket for a couple of day"
(because I did not got time to make such second pass yet).

Consider sending a V2 with more explanation if you want to speedup the
handling of that series.
Sean Farley - June 30, 2016, 3:08 a.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 06/22/2016 01:32 AM, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean@farley.io>
>> # Date 1464306370 25200
>> #      Thu May 26 16:46:10 2016 -0700
>> # Node ID 01212d1f93a2a6c5c736dc0c7d45ffd1930e5c2f
>> # Parent  aa1d56003872cba207d908706da059141dd901a5
>> # EXP-Topic autoverb
>> histedit: move autoverb logic from torule to ruleeditor
>
> This series is taking me more time to review than it could. It mostly
> come from the fact that the commit description are not very helpful to me.
>
> The descriptions in this series are extremely factual and descriptive
> about the patch and does not contains much context. In practice, this
> mean than the description mostly duplicate the patch information without
> helping me to understand why the change is the right thing to do.
>
> For example, on this first changeset, you are moving logic X from class
> A to class B. It would help me to know what is the semantic of class A
> and B and why X fit better in B. Otherwise reviewing this series
> requires me to re-analyze the whole source of histedit in order to
> understand what is going on here. If you could write down the principle
> you used decide how to move forward, it would allow me (and any other
> reader) to first quickly validate the principle and then validate the
> change based on that principle. Because of this, that series have been
> delayed in my "second pass in more depth bucket for a couple of day"
> (because I did not got time to make such second pass yet).
>
> Consider sending a V2 with more explanation if you want to speedup the
> handling of that series.

Sure, makes sense.
Augie Fackler - June 30, 2016, 4:37 p.m.
On Wed, Jun 29, 2016 at 08:08:59PM -0700, Sean Farley wrote:
> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>
> > On 06/22/2016 01:32 AM, Sean Farley wrote:
> >> # HG changeset patch
> >> # User Sean Farley <sean@farley.io>
> >> # Date 1464306370 25200
> >> #      Thu May 26 16:46:10 2016 -0700
> >> # Node ID 01212d1f93a2a6c5c736dc0c7d45ffd1930e5c2f
> >> # Parent  aa1d56003872cba207d908706da059141dd901a5
> >> # EXP-Topic autoverb
> >> histedit: move autoverb logic from torule to ruleeditor
> >
> > This series is taking me more time to review than it could. It mostly
> > come from the fact that the commit description are not very helpful to me.
> >
> > The descriptions in this series are extremely factual and descriptive
> > about the patch and does not contains much context. In practice, this
> > mean than the description mostly duplicate the patch information without
> > helping me to understand why the change is the right thing to do.
> >
> > For example, on this first changeset, you are moving logic X from class
> > A to class B. It would help me to know what is the semantic of class A
> > and B and why X fit better in B. Otherwise reviewing this series
> > requires me to re-analyze the whole source of histedit in order to
> > understand what is going on here. If you could write down the principle
> > you used decide how to move forward, it would allow me (and any other
> > reader) to first quickly validate the principle and then validate the
> > change based on that principle. Because of this, that series have been
> > delayed in my "second pass in more depth bucket for a couple of day"
> > (because I did not got time to make such second pass yet).
> >
> > Consider sending a V2 with more explanation if you want to speedup the
> > handling of that series.
>
> Sure, makes sense.

I agree that the log messages here could probably be better, but the
proposed functionality looks great to me, and the implementation seems
fine (so when a V2 lands, it should be a breeze).

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -421,18 +421,10 @@  class histeditaction(object):
         """
         ctx = self.repo[self.node]
         summary = ''
         if ctx.description():
             summary = ctx.description().splitlines()[0]
-
-        fword = summary.split(' ', 1)[0].lower()
-        # if it doesn't end with the special character '!' just skip this
-        if (self.repo.ui.configbool("experimental", "histedit.autoverb") and
-            initial and fword.endswith('!')):
-            fword = fword[:-1]
-            if fword in primaryactions | secondaryactions | tertiaryactions:
-                self.verb = fword
         line = '%s %s %d %s' % (self.verb, ctx, ctx.rev(), summary)
         # trim to 75 columns by default so it's not stupidly wide in my editor
         # (the 5 more are left for verb)
         maxlen = self.repo.ui.configint('histedit', 'linelen', default=80)
         maxlen = max(maxlen, 22) # avoid truncating hash
@@ -1315,10 +1307,24 @@  def between(repo, old, new, keep):
 def ruleeditor(repo, ui, actions, editcomment=""):
     """open an editor to edit rules
 
     rules are in the format [ [act, ctx], ...] like in state.rules
     """
+    if repo.ui.configbool("experimental", "histedit.autoverb"):
+        for act in actions:
+            ctx = repo[act.node]
+            summary = ''
+            if ctx.description():
+                summary = ctx.description().splitlines()[0]
+
+            fword = summary.split(' ', 1)[0].lower()
+            # if it doesn't end with the special character '!' just skip this
+            if fword.endswith('!'):
+                fword = fword[:-1]
+                if fword in primaryactions | secondaryactions | tertiaryactions:
+                    act.verb = fword
+
     rules = '\n'.join([act.torule(initial=True) for act in actions])
     rules += '\n\n'
     rules += editcomment
     rules = ui.edit(rules, ui.username(), {'prefix': 'histedit'})