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

Jean Jordaan jean.jordaan at gmail.com
Thu Feb 11 10:48:03 UTC 2010


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)))

>>   <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.

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.

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"
     />

(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"
     />

(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__"
     />

(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"
     />

(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"
     />

(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 this).

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.

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.

>> 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.

> 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.

>> 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 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.

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).

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

-- 
jean                                              . .. .... //\\\oo///\\




More information about the Product-Developers mailing list