Code review comment for lp:~cerberus/nova/nova_notifications

Revision history for this message
Matt Dietz (cerberus) wrote :

Thanks for the feedback, guys.

Jay: I think I've cleaned up what you suggested.

Brian: I made the message changes

I'm a little confused by the request to implement a base class here. Each notifier only has a single argument "notify" method(but the self reference), and that's all there would be to document amongst them. The functionality after that point is entirely different. Beyond that, I feel like the api.notify method is the real one to watch.

Regarding the idea of base classes, I'm opposed to the practice of implementing interfaces in Python. Given the "duck typing" nature of Python, it seems superfluous. If there was methods or member variables to be shared amongst them, it would of course be different.

I've noticed the practice in Nova is starting to lean the way of implementing interfaces, but the argument that's been made is that it's intended to catch missing functionality from driver classes. However, the right answer, in my opinion, is to ensure you've written tests to that implicit interface.

« Back to merge proposal