[PATCH 1 of 9] histedit: use "editor" argument of "commit()" instead of explicit "ui.edit()"
Pierre-Yves David
pierre-yves.david at ens-lyon.org
Wed May 7 18:10:41 UTC 2014
On 05/07/2014 07:17 AM, FUJIWARA Katsunori wrote:
>
> At Tue, 06 May 2014 17:14:30 -0700,
> Pierre-Yves David wrote:
>>
>> On 05/05/2014 05:33 AM, FUJIWARA Katsunori wrote:
>>> # HG changeset patch
>>> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
>>> # Date 1399292800 -32400
>>> # Mon May 05 21:26:40 2014 +0900
>>> # Branch stable
>>> # Node ID b683de8e06581ebb2a59120b118539a839bcfb06
>>> # Parent cadad384c97c7956c98f3c9b92d8cc40fa16d93b
>>> histedit: use "editor" argument of "commit()" instead of explicit "ui.edit()"
>>
>> After a long hesitation I ended up queuing this series to the
>> clowncopter repo.
>>
>> I uncomfortable with multiple aspect of this series and I'm half
>> thinking that passing editor to memctx.__init__ is a layer violation
>> (but also half thinking it make perfect sense).
>>
>> However I finally realized that nothing in this series can be worth that
>> the widespread new._test = editor(…) idioms.
>>
>> Thanks alot for the cleanup!
>>
>> Consider keep digging further in this direction.
>
> In fact, I already finished to work for patches below:
>
> - replace explicit "opts['edit']" checking and choosing
> "commit*editor" by specific utility function to simplify code path
>
> before:
> editor = cmdutil.commiteditor # or editor = None/False
> if opts.get('edit'):
> editor = cmdutil.commitforceeditor
>
> after:
> editor = cmdutil.getcommiteditor(**opts)
>
> - replace explicit "ui.edit()" using by newly added utility function
>
> before:
> def editor(repo, ctx, subs):
> return ui.edit(ctx.description() + "\n", ctx.user())
>
> after:
> editor = cmdutil.getcommiteditor(edit=True)
>
> but didn't include them into this series to keep this series small
> enough, because above consists of over 10 patches :-)
>
> Then, are there any other refactoring points you suppose ?
Yes
1. first I over looked the lack of doc in your memctx change. I would be
dashed cool if you could update the function documentation explaining:
* What are the expected prototype this ancestors function
* its expected behavior
* its interaction with the `text` argument of the same function
* Why it is necessary for this function to be there
2. getting savecommitmessage out of local repo would be a plus
3. I;m stil not super fan of this function passed to __init__ but I
can't think of anything better for now.
Cheers,
--
Pierre-Yves David
More information about the Mercurial-devel
mailing list