[PATCH 1 of 2] help: extract help text generation into a separate function

Mads Kiilerich mads at kiilerich.com
Sat Jun 30 20:03:57 CDT 2012


Olav Reinert wrote, On 06/30/2012 11:20 PM:
> On 30. jun. 2012, at 23.13, Sune Foldager wrote:
>
>> On 2012-06-26 23:23, Olav Reinert wrote:
>>> # HG changeset patch
>>> # User Olav Reinert <seroton10 at gmail.com>
>>> # Date 1340745489 -7200
>>> # Node ID 77d124b770d077179d3e55c48eadc141d2fc7740
>>> # Parent  cd7db8e05c657fb08b51903fa2ef8c2246de1c4b
>>> help: extract help text generation into a separate function
>>>
>>> The help command is refactored so that all generation of RST help text is
>>> encapsulated in a separate function (called 'helprst'). Because the intention
>>> is to move helprst into help.py, a number of small changes (which make no
>>> semantic difference) are made in preparation for the move. In this way, the
>>> move itself is a pure cut-and-paste of a large text block, without any other
>>> changes.
>> [...]
>>
>>> +from mercurial.help import *
>> For what it's worth, I'd prefer not doing this, as it can be problematic and
>> is not considered good style, and instead just fixing a few more things up
>> after the code is moved.

Even more notable is adding some "import commands # avoid cycle" to 
commands.py.

(Adding the commands imports to help.py makes more sense, but adding 
more delayed imports to work around a cyclic issue seems like a step in 
the wrong direction. There might however not be a better solution.)

> Yes. The reason I did it anyway is that I somehow got the impression that Matt dislikes changes combined with moving large blocks of code around. But perhaps fixing only namespace-related issues is OK in such cases?

I would say yes. I would review such refactorings by naively redoing (or 
undoing) moved blocks and see what the resulting diff is. Namespacing 
fixes would be easy to spot and not be a problem. Other refactorings or 
other changes done while moving code around would however be very hard 
to spot when looking at code history later on - they should be done in 
seperate patches.

But FWIW I'm +/- 0 on these patches. I have no doubt that they make a 
lot of sense as preparation for something you are working on, but as it 
is now they don't add any value upstream and I can't argue why they 
should be applied now. You or someone else could end up with another 
solution that is going in a different direction.

I suggest to postpone these patches and post them again as a part of the 
patchbomb when you start utilizing these refactorings.

Others might disagree and like the value the patches add right now and 
accept them ;-)

/Mads


More information about the Mercurial-devel mailing list