Code review comment for lp://qastaging/~danilo/simplestreams/insert-item-factoring

Revision history for this message
Данило Шеган (danilo) wrote :

Thanks for the review, Scott.

What worries you about the changed signature of GlanceMirror? It's an optional parameter put at the very end, thus no existing call-site could be affected. Dependency injection of "OpenStack module" like this allows for simpler testing without having to re-factor the constructor itself (which does too much anyway). Alternative is to move any calls to openstack.* and whatever is using the returned data outside the constructor, and call that in every method which relies on actual connections (thus following the ideology of only setting up the data in the constructor, but not really doing anything), but that's a bit outside the scope of this branch.

As for hypervisor_mapping, it could very well be at the class level since we initialize GlanceMirror with the config object. However, for helper methods themselves, I prefer to have them rely as little as possible on their "context": makes it clear what their inputs and outputs are (and they are easier to test as well). Thus, I've changed nothing in this regard, but if you prefer it any other way, just shout. :)

I've done the variable renames and pushed that up.

« Back to merge proposal