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

Martin Aspeli optilude+lists at gmail.com
Thu Feb 11 12:59:09 UTC 2010


Jean Jordaan wrote:
> Hi there
>
>> Yep. What I'm saying is that if you didn't specify an original, then
>> ignoreOriginal shouldn't be needed either.
>
> Ah. At the moment, however, 'ignoreOriginal' is needed, because if you
> don't specify 'original', you get::
>
>      ConfigurationError: ('Missing parameter:', 'original')
>
> and if you don't specify 'ignoreOriginal', you get::
>
>      if to_be_replaced is None and not ignoreOriginal:
>          raise ConfigurationError("Original %s in %s not found" %
> (original, str(scope)))

Yes, I'm saying we should fix that. ignoreOriginal is a bit stupid (but 
we'd need it for BBB). We should just say that if you specify an 
original it's an error if it's not there, and if you don't, it's an 
error if it *is* there.

>>     <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.
>
> The part I don't like is that in this case the patch is supplied in the
> 'new' attribute, while otherwise it's supplied in the 'patch' attribute.
> I'd rather always have the patch in the 'patch' attribute.

Yeah, I can buy that argument. The above with just 'patch' instead of 
'new' is OK, too.

> My proposal in overview:
>
> (1) Replace existing member (common case, exception if
> 'readRemoteProject' is absent)::
>
>    <monkey:patch
>       description="Add XML-RPC capability to SomeModule"
>       module="Products.SomeProduct.SomeModule"
>       patch=".SomeModule.readRemoteProject"
>       />

-1

As I've said, if you are *replacing* you have to say what you're 
replacing. Implying a replace if a function with the same name as the 
patch is found is unacceptable.

So replace existing member should be:

<monkey:patch
     description="..."
     module="foo.bar"
     original="name"
     patch="whatever"
     />

In this case, it is an error if 'original' is not found


> (2) Inject new member (exception if 'readRemoteProject' is present)::
>
>    <monkey:patch
>       description="Add XML-RPC capability to SomeModule"
>       module="Products.SomeProduct.SomeModule"
>       new="True"
>       patch=".SomeModule.readRemoteProject"
>       />

-0

I think would prefer:

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

Though I don't care very much either way.

And indeed, an error if a method with the same name already exists.

> (3) Replace existing member using differently named patch (exception if
> 'readRemoteProject' is absent)::
>
>    <monkey:patch
>       description="Add XML-RPC capability to SomeModule"
>       module="Products.SomeProduct.SomeModule"
>       original="readRemoteProject"
>       patch=".SomeModule.readRemotePorject__"
>       />

+1, but this is the same as (1).

> (4) You don't like (1), so it becomes (extra 'original' line,
> required)::
>
>    <monkey:patch
>       description="Add XML-RPC capability to SomeModule"
>       module="Products.SomeProduct.SomeModule"
>       original="readRemoteProject"
>       patch=".SomeModule.readRemoteProject"
>       />

Indeed.

> (5) Your suggestion for injecting a new member (2): ('patch' becomes
> 'new', absence check implied)::
>
>    <monkey:patch
>       description="Add XML-RPC capability to SomeModule"
>       module="Products.SomeProduct.SomeModule"
>       new=".SomeModule.readRemoteProject"
>       />

Yep, but I'm not too fussed.

> (6) For consistency's sake, it should really be something like::
>
>    <monkey:patch
>       description="Add XML-RPC capability to SomeModule"
>       module="Products.SomeProduct.SomeModule"
>       new=".SomeModule.readRemoteProject"
>       as="readRemoteProject"
>       />
>
> .. because why should only a replacement, and not a new member, be named
> explicitly? (I'd prefer to make differently named replacements an
> aberration, and not support t/me his).

/me shrugs

I think the "as" should be optional, at least.

I think the "replace" and "inject" use cases are different. I don't mind 
if "inject" by default uses the patch method name as the new method 
name, so long as you get an error if the item is already there (in all 
"new" cases). I do mind if "replace" assumes they are the same.

> I would be fine with (2) (3) (4) as an improvement on the current way of
> adding a new member. I don't like changing from 'patch' to 'new' in (5)
> and I don't think (6) is terribly useful.

Agree.

> Unless you're really interested in this debate, please don't bother to
> read beyond this line :-)
>
>> 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.
>
> You are. You can't use this 'patch' to replace anything except a thing
> with the same name, unless you explicitly ask for it. I agree that you
> should say exactly what you're replacing, but not that you should say it
> twice.
>
> I appreciate your point that the "patch" is referring to an entirely
> different method with a different module path. I just want to assert
> that normally the patcher can name that method, has created it for one
> reason only and that's to replace a specific method, will name it
> sensibly, and that the obvious name is that of the method it's
> replacing.

I rarely use the same name in my patches, preferring something a bit 
more explicit so people don't accidentally think they're meant to call it.

>>> 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.
>
> I hate it when a system makes me type something and then mocks me when I
> mistype it (or miss-copy, or miss-autocomplete), instead of just not
> making me type it in the first place.

/me shrugs

>> 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.
>
> Yes, I'm suggesting that this product should subscribe to that
> convention. Part of its raison d'etre is to establish sensible
> conventions. And I just cannot imagine that the majority of replacement
> methods would be named *differently* from the originals. So I think it
> will be a repeat in the majority of cases.

The majority of mine are. Anyway, insofar as I maintain 
collective.monkeypatcher, the idea of an automatic match for the 
"replace" use case based on method name is rejected. Unless more people 
strongly voice in favour, then it's not going in, for the reason outlined.

>>> 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.
>
> What could they possibly break? It's no error to specify an 'original'
> that's the same as the patch name.

I want people reading the code to very clearly understand that there is 
a monkey patch in effect, what exactly is being patched, and why. If I 
read this:

<monkey:patch
    description="Improve mail handling"
    class="Products.MailHost.MailHost.MailHost"
    patch=".patches.send"
    />

I would assume it was patching a new method onto that class. I wouldn't 
have thought it was looking for a method called "send" just because you 
had stated ".patch.send" as the patch method, and that it was replacing 
this.

>> 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.
>
> I think it's also important that ctags finds my patch-method as well as
> the original method, and as I've argued I don't think my proposal loses
> anything in terms of exactness. I don't find that being forced to
> specify something twice makes it any more exact. The convention of
> naming patches the same as originals makes it more clear what you're
> replacing.

/me shrugs

Hopefully you're not doing this often that it'll actually matter.

> So I completely agree with what you're saying, and I think my proposal
> is even closer to that than yours! :-)
>
>> If you're doing it so much that re-typing this is causing you pain,
>> you have no sympathy from me.
>
> It's not only my patches, it's all future patches I may have to deal
> with. And it's not only retyping, it's also reading the source of the
> patching product and using ctags (or equivalents).

My patches tend to be called e.g.

ActionTool_send()

or something like that. I think this is more clear. The point about 
ctags is a good one (I don't use ctags myself), though. I'm not opposed 
to the patch having the same name. I just don't think it's sufficient.

> This particular case really isn't that important, but I think the issues
> it raises are worth discussing.

Sure.

Martin

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





More information about the Product-Developers mailing list