| 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
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.
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.
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'})