Closed DanielSank closed 1 year ago
This is a very useful feature. You should add proper tests, though. You'll certainly run into Python 2 vs 3 compatibility issues due to the change in metaclass syntax. There is a precedent for that in the test/test_signature.py
module.
I am not sure how to handle the docstring here. Conveniently omitting the prompt >>>
makes the test suite ignore it, but that's a dangerous thing to do, since it introduces the possibility of having disfunctional code samples in the docstring.
This raises the general issue of how to deal with cross-version compatibility. Thus I'd suggest you to get to some point where it "just works" including tests. We can proceed to a more general policy later.
I may need a little help here. I don't understand how the tests are run. In my own checkout I added
if __name__ == "__main__":
unittest.main()
but I'm clearly missing something.
If you install the nose package you can run the tests with 'nosetests' from the base directory.
On February 22, 2014 8:43:55 PM CET, Daniel Sank notifications@github.com wrote:
I may need a little help here. I don't understand how the tests are run. In my own checkout I added
if name == "main": unittest.main()
but I'm clearly missing something.
Reply to this email directly or view it on GitHub: https://github.com/aepsil0n/obsub/pull/43#issuecomment-35812298
Note: you can do python{2,3} compatible instanciation of classes with metaclass like so:
class Meta(type):
....
# use the metaclass to instanciate a new class with no bases () and empty clsdict {}:
Base = Meta('Base', (), {})
# Foo inherits the metaclass from Base:
class Foo(Base):
def foo(self):
pass
Of course you could also instanciate Foo directly, but often times this is much less convenient..
I'm not sure about the usefulness of the metaclass..
On the other hand that's probably the user's problem. I wouldn't use it :P, but if you want to add it, go ahead.
I have a minor comment regarding the implementation: The try: ...; except KeyError: ..
can have a KeyError
even if d['_event_methods']
is present (if there is an overload for getattr(base, m).__func__
). So better change it to:
try:
event_methods = clsdict['_event_methods']
except KeyError:
raise RuntimeError("...")
else:
for m in event_methods:
...
or just let the exception go unhandled (I think the KeyError is probably descriptive enough).
btw, I think that the logic in the docstring is suited for testing. You just have to generalize it for Python 2.x and 3.x, e.g. as @coldfix suggested. The coverage went down, because you only defined test classes in the tests but did no instantiation and testing.
Suppose you have a class Foo which has a method .bar to which you'd like to apply the @event decorator, but that class is defined externally to your project. You're stuck subclassing Foo and re-implementing .bar with manual addition of the @event decorator. That's ok for some cases, but it's kind of ugly and can be really tedious.
In this submission I introduce a metaclass so that the @event decorator can be applied to specified methods at class construction time. A full explanation with a working example are given in the docstring of obsub.EventMetaclass, but here's the basic idea