[Product-Developers] Re: collective.monkeypatcher -- awkward names

[Martin Aspeli]

Jean Jordaan wrote:
> Hi Martin&  all
>
> Current:
>
>>>   <monkey:patch
>>>      description="Add XML-RPC capability to SomeModule"
>>>      module="Products.SomeProduct.SomeModule"
>>>      original="readRemoteProject"
>>>      ignoreOriginal="True"
>>>      replacement=".SomeModule.readRemoteProject"
>>>      />
>
> Suggested:
>
>>>   <monkey:patch
>>>      description="Add XML-RPC capability to SomeModule"
>>>      module="Products.SomeProduct.SomeModule"
>>>      checkOriginal="False"
>>>      patch=".SomeModule.readRemoteProject"
>>>      />
>> +1
>>
>> I'm not sure what ignoreOriginal is doing in this case; I'd probably just
>> ignore it.
>
> In the original, 'ignoreOriginal="True"' is required because the
> original doesn't exist. I'm adding a new function. If you ignore it,
> you'll get an exception.

Yep. What I'm saying is that if you didn't specify an original, then 
ignoreOriginal shouldn't be needed either. So the way to add a new 
function to a module is:

<monkey:patch
   description="..."
   module="foo.bar"
   patch=".new.function"
   />

> In my suggested version, you have to switch off the existence check that
> 'patch' normally does, because in this case you know that there is no
> original. That said, I'd like to suggest 'new' instead of
> 'checkOriginal'::
>
>    <monkey:patch
>       description="Add XML-RPC capability to SomeModule"
>       module="Products.SomeProduct.SomeModule"
>       new="True"
>       patch=".SomeModule.readRemoteProject"
>       />

Yeah, that's even more explicit, which is probably good. Or how about just:

     <monkey:patch
        description="Add XML-RPC capability to SomeModule"
        module="Products.SomeProduct.SomeModule"
        new=".SomeModule.readRemoteProject"
        />

I think I like that better. It's explicit and less typing.

>>> If I simply want to replace an existing function of the same name,
>>> checking for the original::
>>>
>>>   <monkey:patch
>>>      description="Add XML-RPC capability to SomeModule"
>>>      module="Products.SomeProduct.SomeModule"
>>>      patch=".SomeModule.readRemoteProject"
>>>      />
>> -1
>>
>> I think you should have to explicitly say what you're patching. Just
>> looking for something with the same name is too magical. The whole
>> point of collective.monkeypatcher is to make everything very explicit.
>
> It is explicit; there's no magic. The documentation of 'patch' will say
> that it expects to find a member of the same name. If that member
> doesn't exist, monkeypatcher will raise an exception. That will
> encourage keeping the names of the patch and the original the same,
> which will make the patching product more readable and amenable to e.g.
> ctags, because the names from the original API are preserved.

Sorry, I still don't like it. I think we can encourage people to use the 
same name, but I think that if you want to replace something, you should 
explicitly say both the name of the thing you're replacing and the name 
of the thing you're replacing it with.

>>> If I'm replacing an existing function, and for some reason the
>>> 'description' attribute is insufficient to describe what I'm doing and I
>>> want to name my method something different::
>>>
>>>   <monkey:patch
>>>      description="Add XML-RPC capability to SomeModule"
>>>      module="Products.SomeProduct.SomeModule"
>>>      original="readRemoteProject"
>>>      patch=".SomeModule.readRemoteProject_myownspecialweirdname"
>>>      />
>> This is the current basic syntax, no? I think this is the appropriate way
>> still for both this and the case above where the patch function and the
>> original function happened to have the same name.
>
> Yes, this is like the current. I think the repeat is makework and a
> chance for typos though.

It's validated, so that's not really an issue. And it's not a repeat. 
It's only a repeat if you happen to subscribe to the convention that the 
two methods must have the same name, which they don't. The "patch" is 
referring to an entirely different method with a different module path. 
Only the last bit (the function name) is the same, except it doesn't 
need to be the same.

> I hope you agree that documentation like "by
> default, 'patch' replaces a member of the same name on the patched
> object" makes things clear enough.

I don't. :) Well, I do, but it assumes that people reading the 
<monkey:patch /> statement has read and recall the documentation. I'd 
rather the statement was explicit.

> Of course, I may be completely missing something that invalidates my
> proposal .. please tell me if I am!

I think it's mostly just a matter of balance. If you are monkey 
patching, I think you deserve to spell out very, very carefully exactly 
what you're replacing. If you're doing it so much that re-typing this is 
causing you pain, you have no sympathy from me.

Martin


-- 
Author of `Professional Plone Development`, a book for developers who
want to work with Plone. See http://martinaspeli.net/plone-book