Allow Registering and unregistering instance methods as listeners

[dwt]

Fixes #12 - registering instance methods as listeners doesn’t allow to easily unregister them as the registry tries to find the handler with 'is' but it should use '==' to allow the python BoundMethod wrapper (which is a new instance every time instance.$methodname is accessed).

[mgedmin]

LGTM

[mgedmin]

This is the line that is not being executed, which causes a test coverage regression. You could drop a # pragma: nocover comment next to it, or use @jamadden's favorite trick of replacing the pass with a docstring, e.g. """example subscriber that is a bound method""" or something.

[jamadden]

Or my other favorite trick, raising an exception if it's called. The best one here would be self.fail("Example method, not intended to be called.")

[dwt]

[x] done

[jamadden]

Personally I would prefer to see this go the other way: replace the underlying use of == with is. The pattern to unregister an object is:

item = registry.getUtility(IWhatever, name='whatever')
registery.unregisterUtility(IWhatever, item, name='whatever')

I think it's rare to already know ahead of time an expression that computes an object that you think is already registered as a utility.

An advantage of is over ==, aside from speed is that is doesn't unghost persistent objects. In some cases, unghosting them may not even be possible (POSKeyError) so is can be very valuable.

[mgedmin]

The problem with using is is that this makes it difficult to unregister bound methods.

[jamadden]

The problem with using is is that this makes it difficult to unregister bound methods.

Sure, if you rely on using that expression every time (and thus creating the new object). If you first retrieve the utility and then unregister it, you don't have a problem. That seems like the more general case to me. Being able to create a new object and unregister based on equality seems like a special case; it may not even make sense to support that special case given persistent objects, and the potential danger of false-positives.

Now, I understand that #12 was not actually about utilities but about subscription adapters or handlers. It is harder to first retrieve a registered handler than it is a utility (but not impossible), so that option still exists. Keeping a reference to the registered object is also a possibility and probably easier. But as for being able to register bound methods as a handler: isn't that a bit of a foot-gun already? They definitely don't work in persistent registries since they can't be pickled:

Python 2.7.13 (default, May  5 2017, 09:16:47)
[GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.38)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo(object):
...     def thing(self): pass
...
>>> f = Foo()
>>> t = f.thing
>>> t
<bound method Foo.thing of <__main__.Foo object at 0x101e82dd0>>
>>> import cPickle
>>> cPickle.dumps(t)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/lib/python2.7/copy_reg.py", line 70, in _reduce_ex
    raise TypeError, "can't pickle %s objects" % base.__name__
TypeError: can't pickle instancemethod objects
>>> cPickle.dumps(t, 2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
cPickle.PicklingError: Can't pickle <type 'instancemethod'>: attribute lookup __builtin__.instancemethod failed

If the use for bound-methods-as-listeners is for test cases and zope.components global registery, then normal test tearDown should already be cleaning them up.

I'm not really strongly opposed to this and won't object to seeing it merged (the main benefit for persistent objects would require another change), I'm just struggling to understand the reasons to support it.

[mgedmin]

I stumbled upon this in 2010 and I cannot for the life of me remember why I was trying to register/unregister bound methods.

I'm okay with saying 'don't do that', as long as the underlying zope.interface and the wrapper zope.component methods are in agreement (which they currently are not).

[dwt]

An advantage of is over ==, aside from speed is that is doesn't unghost persistent objects. In some cases, unghosting them may not even be possible (POSKeyError) so is can be very valuable.

I think the point here is that {un}registerHandler checks if the handler is registered with == but then unregister uses is to find the actual handler - this disagreement causes unregisterHandler to return true (and trigger unghosting), while the actual handler is not unregistered. This is the bug that this pull request tries to fix.

[icemac]

@dwt Is there still something to be done to get this PR ready for merge?