-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added EventMetaclass #43
Conversation
This allows application of the @event decorator to methods defined in a superclass.
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 I am not sure how to handle the docstring here. Conveniently omitting the prompt 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. |
options | ||
|
||
1. Add the @event decorator in Foo. This is a bad option because it | ||
requires us to modify someone elses class. This may not be possible or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elses -> else's
Fixed spelling mistakes and made exception more concise.
I may need a little help here. I don't understand how the tests are run. In my own checkout I added
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:
|
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:
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