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

Revision history for this message
Brian Waldon (bcwaldon) wrote :

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

You're definitely right, and the reason I suggested it was more to help with documentation rather than enforcing interfaces. It took me longer than it should have to wrap my head around the system as a whole, and I thought it would help others get up to speed quicker. It really isn't a big deal, just an idea I wanted to run by you. I am ready to approve once the following are addressed:

78: you don't seem to use 'event_name', 'message' should be 'payload'

84: this line doesn't belong

90: 'message' should be 'payload'

92: remove 'payload'

103: 'message' should be 'payload'

114: 'message' should be 'payload'

review: Needs Fixing

« Back to merge proposal