Patchwork [1,of,3] filemerge: split the logic for finding an external tool to its own function

login
register
mail settings
Submitter Matt Harbison
Date Nov. 2, 2014, 8:58 p.m.
Message ID <f72827d6b96cb953432f.1414961901@Envy>
Download mbox | patch
Permalink /patch/6533/
State Accepted
Headers show

Comments

Matt Harbison - Nov. 2, 2014, 8:58 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1414958330 18000
#      Sun Nov 02 14:58:50 2014 -0500
# Branch stable
# Node ID f72827d6b96cb953432fff89a0bd3b13a142e826
# Parent  cc1cbb0bba8ed1d95c8f1b8e27d4d2893e0dcca7
filemerge: split the logic for finding an external tool to its own function

This will be used by extdiff in an subsequent patch.
Pierre-Yves David - Nov. 2, 2014, 9:46 p.m.
On 11/02/2014 08:58 PM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1414958330 18000
> #      Sun Nov 02 14:58:50 2014 -0500
> # Branch stable
> # Node ID f72827d6b96cb953432fff89a0bd3b13a142e826
> # Parent  cc1cbb0bba8ed1d95c8f1b8e27d4d2893e0dcca7
> filemerge: split the logic for finding an external tool to its own function
>
> This will be used by extdiff in an subsequent patch.
>
> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
> --- a/mercurial/filemerge.py
> +++ b/mercurial/filemerge.py
> @@ -37,6 +37,9 @@
>   def _findtool(ui, tool):
>       if tool in internals:
>           return tool
> +    return findexternaltool(ui, tool)
> +
> +def findexternaltool(ui, tool):
>       for kn in ("regkey", "regkeyalt"):
>           k = _toolstr(ui, tool, kn)
>           if not k:

The docstring deiti would be pleased to see one. And you will be happy 
to not see this function being dropped in 6 month because it had no 
obvious other caller and no explanation of its purpose in life ;-)

I like where this series is going otherwise.
Matt Harbison - Nov. 3, 2014, 12:12 a.m.
Pierre-Yves David wrote:
>
>
> On 11/02/2014 08:58 PM, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1414958330 18000
>> # Sun Nov 02 14:58:50 2014 -0500
>> # Branch stable
>> # Node ID f72827d6b96cb953432fff89a0bd3b13a142e826
>> # Parent cc1cbb0bba8ed1d95c8f1b8e27d4d2893e0dcca7
>> filemerge: split the logic for finding an external tool to its own
>> function
>>
>> This will be used by extdiff in an subsequent patch.
>>
>> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
>> --- a/mercurial/filemerge.py
>> +++ b/mercurial/filemerge.py
>> @@ -37,6 +37,9 @@
>> def _findtool(ui, tool):
>> if tool in internals:
>> return tool
>> + return findexternaltool(ui, tool)
>> +
>> +def findexternaltool(ui, tool):
>> for kn in ("regkey", "regkeyalt"):
>> k = _toolstr(ui, tool, kn)
>> if not k:
>
> The docstring deiti would be pleased to see one. And you will be happy
> to not see this function being dropped in 6 month because it had no
> obvious other caller and no explanation of its purpose in life ;-)
>
> I like where this series is going otherwise.
>
>

OK, easy enough.  I thought about it, but didn't because it is doing 
less than the undocumented function it was split from.  Is that the 
secondary purpose of docstring- i.e. indicate that it might be used 
elsewhere?  I can't recall seeing one that explicitly said it was used 
elsewhere.

I'll wait a day and see if anyone else has feedback before resending.

--Matt

Patch

diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -37,6 +37,9 @@ 
 def _findtool(ui, tool):
     if tool in internals:
         return tool
+    return findexternaltool(ui, tool)
+
+def findexternaltool(ui, tool):
     for kn in ("regkey", "regkeyalt"):
         k = _toolstr(ui, tool, kn)
         if not k: