Patchwork [1,of,2] histedit: extract method ruleeditor

login
register
mail settings
Submitter Mateusz Kwapich
Date Feb. 18, 2015, 12:52 a.m.
Message ID <494a60ee7b222940db7f.1424220738@dev1429.prn1.facebook.com>
Download mbox | patch
Permalink /patch/7804/
State Superseded
Commit 5a64b676c5d3f3a0692b7fa1414e702f7231e133
Headers show

Comments

Mateusz Kwapich - Feb. 18, 2015, 12:52 a.m.
# HG changeset patch
# User Mateusz Kwapich <mitrandir@fb.com>
# Date 1421952770 28800
#      Thu Jan 22 10:52:50 2015 -0800
# Node ID 494a60ee7b222940db7f95c0546ada91d13e180c
# Parent  ff5caa8dfd993680d9602ca6ebb14da9de10d5f4
histedit: extract method ruleeditor

Extract functionality of editing histedit rules to separate method so we can
reuse it in upcoming --edit-plan option.
Ryan McElroy - Feb. 18, 2015, 6:48 p.m.
On 2/17/2015 4:52 PM, Mateusz Kwapich wrote:
> # HG changeset patch
> # User Mateusz Kwapich <mitrandir@fb.com>
> # Date 1421952770 28800
> #      Thu Jan 22 10:52:50 2015 -0800
> # Node ID 494a60ee7b222940db7f95c0546ada91d13e180c
> # Parent  ff5caa8dfd993680d9602ca6ebb14da9de10d5f4
> histedit: extract method ruleeditor
>
> Extract functionality of editing histedit rules to separate method so we can
> reuse it in upcoming --edit-plan option.
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -638,16 +638,9 @@
>   
>           ctxs = [repo[r] for r in revs]
>           if not rules:
> -            rules = '\n'.join([makedesc(c) for c in ctxs])
> -            rules += '\n\n'
> -            rules += editcomment % (node.short(root), node.short(topmost))
> -            rules = ui.edit(rules, ui.username())
> -            # Save edit rules in .hg/histedit-last-edit.txt in case
> -            # the user needs to ask for help after something
> -            # surprising happens.
> -            f = open(repo.join('histedit-last-edit.txt'), 'w')
> -            f.write(rules)
> -            f.close()
> +            rules = ruleeditor(repo, ui, [['pick', c] for c in ctxs],
> +                               editcomment=editcomment % (node.short(root),
> +                                                          node.short(topmost)))
>           else:
>               if rules == '-':
>                   f = sys.stdin
> @@ -805,20 +798,40 @@
>               raise util.Abort(_('cannot edit immutable changeset: %s') % root)
>       return [c.node() for c in ctxs]
>   
> -def makedesc(c):
> -    """build a initial action line for a ctx `c`
> +def makedesc(repo, action, rev):
> +    """build a initial action line for a ctx
>   
>       line are in the form:
>   
> -      pick <hash> <rev> <summary>
> +      <action> <hash> <rev> <summary>
>       """
> +    ctx = repo[rev]
>       summary = ''
> -    if c.description():
> -        summary = c.description().splitlines()[0]
> -    line = 'pick %s %d %s' % (c, c.rev(), summary)
> +    if ctx.description():
> +        summary = ctx.description().splitlines()[0]
> +    line = '%s %s %d %s' % (action, ctx, ctx.rev(), summary)
>       # trim to 80 columns so it's not stupidly wide in my editor
>       return util.ellipsis(line, 80)
The simultaneous changes to makedesc make this diff harder to parse. 
Could you split those out into their own change? That will make this be 
two trivial refactors instead of one more difficult refactor.

>   
> +def ruleeditor(repo, ui, rules, editcomment=""):
> +    """open an editor to edit rules
> +
> +    rules are in the format [ [act, ctx], ...] like in state.rules
> +    """
> +    rules = '\n'.join([makedesc(repo, act, rev) for [act, rev] in rules])
> +    rules += '\n\n'
> +    rules += editcomment
> +    rules = ui.edit(rules, ui.username())
> +
> +    # Save edit rules in .hg/histedit-last-edit.txt in case
> +    # the user needs to ask for help after something
> +    # surprising happens.
> +    f = open(repo.join('histedit-last-edit.txt'), 'w')
> +    f.write(rules)
> +    f.close()
> +
> +    return rules
> +
>   def verifyrules(rules, repo, ctxs):
>       """Verify that there exists exactly one edit rule per given changeset.
>   
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Feb. 19, 2015, 8:12 p.m.
On Wed, Feb 18, 2015 at 10:48:40AM -0800, Ryan McElroy wrote:
> On 2/17/2015 4:52 PM, Mateusz Kwapich wrote:
> ># HG changeset patch
> ># User Mateusz Kwapich <mitrandir@fb.com>
> ># Date 1421952770 28800
> >#      Thu Jan 22 10:52:50 2015 -0800
> ># Node ID 494a60ee7b222940db7f95c0546ada91d13e180c
> ># Parent  ff5caa8dfd993680d9602ca6ebb14da9de10d5f4
> >histedit: extract method ruleeditor
> >
> >Extract functionality of editing histedit rules to separate method so we can
> >reuse it in upcoming --edit-plan option.
> >
> >diff --git a/hgext/histedit.py b/hgext/histedit.py
> >--- a/hgext/histedit.py
> >+++ b/hgext/histedit.py
> >@@ -638,16 +638,9 @@
> >          ctxs = [repo[r] for r in revs]
> >          if not rules:
> >-            rules = '\n'.join([makedesc(c) for c in ctxs])
> >-            rules += '\n\n'
> >-            rules += editcomment % (node.short(root), node.short(topmost))
> >-            rules = ui.edit(rules, ui.username())
> >-            # Save edit rules in .hg/histedit-last-edit.txt in case
> >-            # the user needs to ask for help after something
> >-            # surprising happens.
> >-            f = open(repo.join('histedit-last-edit.txt'), 'w')
> >-            f.write(rules)
> >-            f.close()
> >+            rules = ruleeditor(repo, ui, [['pick', c] for c in ctxs],
> >+                               editcomment=editcomment % (node.short(root),
> >+                                                          node.short(topmost)))
> >          else:
> >              if rules == '-':
> >                  f = sys.stdin
> >@@ -805,20 +798,40 @@
> >              raise util.Abort(_('cannot edit immutable changeset: %s') % root)
> >      return [c.node() for c in ctxs]
> >-def makedesc(c):
> >-    """build a initial action line for a ctx `c`
> >+def makedesc(repo, action, rev):
> >+    """build a initial action line for a ctx
> >      line are in the form:
> >-      pick <hash> <rev> <summary>
> >+      <action> <hash> <rev> <summary>
> >      """
> >+    ctx = repo[rev]
> >      summary = ''
> >-    if c.description():
> >-        summary = c.description().splitlines()[0]
> >-    line = 'pick %s %d %s' % (c, c.rev(), summary)
> >+    if ctx.description():
> >+        summary = ctx.description().splitlines()[0]
> >+    line = '%s %s %d %s' % (action, ctx, ctx.rev(), summary)
> >      # trim to 80 columns so it's not stupidly wide in my editor
> >      return util.ellipsis(line, 80)
> The simultaneous changes to makedesc make this diff harder to parse. Could
> you split those out into their own change? That will make this be two
> trivial refactors instead of one more difficult refactor.

+1, agree with Ryan's comments enough I'm dropping these and will wait
for a resend.

>
> >+def ruleeditor(repo, ui, rules, editcomment=""):
> >+    """open an editor to edit rules
> >+
> >+    rules are in the format [ [act, ctx], ...] like in state.rules
> >+    """
> >+    rules = '\n'.join([makedesc(repo, act, rev) for [act, rev] in rules])
> >+    rules += '\n\n'
> >+    rules += editcomment
> >+    rules = ui.edit(rules, ui.username())
> >+
> >+    # Save edit rules in .hg/histedit-last-edit.txt in case
> >+    # the user needs to ask for help after something
> >+    # surprising happens.
> >+    f = open(repo.join('histedit-last-edit.txt'), 'w')
> >+    f.write(rules)
> >+    f.close()
> >+
> >+    return rules
> >+
> >  def verifyrules(rules, repo, ctxs):
> >      """Verify that there exists exactly one edit rule per given changeset.
> >_______________________________________________
> >Mercurial-devel mailing list
> >Mercurial-devel@selenic.com
> >http://selenic.com/mailman/listinfo/mercurial-devel
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -638,16 +638,9 @@ 
 
         ctxs = [repo[r] for r in revs]
         if not rules:
-            rules = '\n'.join([makedesc(c) for c in ctxs])
-            rules += '\n\n'
-            rules += editcomment % (node.short(root), node.short(topmost))
-            rules = ui.edit(rules, ui.username())
-            # Save edit rules in .hg/histedit-last-edit.txt in case
-            # the user needs to ask for help after something
-            # surprising happens.
-            f = open(repo.join('histedit-last-edit.txt'), 'w')
-            f.write(rules)
-            f.close()
+            rules = ruleeditor(repo, ui, [['pick', c] for c in ctxs],
+                               editcomment=editcomment % (node.short(root),
+                                                          node.short(topmost)))
         else:
             if rules == '-':
                 f = sys.stdin
@@ -805,20 +798,40 @@ 
             raise util.Abort(_('cannot edit immutable changeset: %s') % root)
     return [c.node() for c in ctxs]
 
-def makedesc(c):
-    """build a initial action line for a ctx `c`
+def makedesc(repo, action, rev):
+    """build a initial action line for a ctx
 
     line are in the form:
 
-      pick <hash> <rev> <summary>
+      <action> <hash> <rev> <summary>
     """
+    ctx = repo[rev]
     summary = ''
-    if c.description():
-        summary = c.description().splitlines()[0]
-    line = 'pick %s %d %s' % (c, c.rev(), summary)
+    if ctx.description():
+        summary = ctx.description().splitlines()[0]
+    line = '%s %s %d %s' % (action, ctx, ctx.rev(), summary)
     # trim to 80 columns so it's not stupidly wide in my editor
     return util.ellipsis(line, 80)
 
+def ruleeditor(repo, ui, rules, editcomment=""):
+    """open an editor to edit rules
+
+    rules are in the format [ [act, ctx], ...] like in state.rules
+    """
+    rules = '\n'.join([makedesc(repo, act, rev) for [act, rev] in rules])
+    rules += '\n\n'
+    rules += editcomment
+    rules = ui.edit(rules, ui.username())
+
+    # Save edit rules in .hg/histedit-last-edit.txt in case
+    # the user needs to ask for help after something
+    # surprising happens.
+    f = open(repo.join('histedit-last-edit.txt'), 'w')
+    f.write(rules)
+    f.close()
+
+    return rules
+
 def verifyrules(rules, repo, ctxs):
     """Verify that there exists exactly one edit rule per given changeset.