Code review comment for lp:~blamar/nova/openstack-api-1-1-images

Revision history for this message
Jay Pipes (jaypipes) wrote :

Hi! Good stuff! A few suggestions from me:

173 + self.__compute = compute_service or compute.API()
174 + self.__image = image_service or _default_service

Please use zero or a single underscore, not two, for attributes. One underscore indicates a "private" attribute/method (even though Python has no concept of a private attribute, really... Generally, two underscores indicate that the attribute/method is special to the system (like __dict__, etc).

154 - "serverId", "progress"]}}}
...
158 + "serverId", "progress"],
159 + "link": ["rel", "type", "href"],
160 + },
161 + },
162 + }

This will break pep8 0.5.0. Just an FYI. If you run your tests with ./run_tests.sh -V, you would see this pop up. Whether pep8 0.6 should be in tools/pip-requires is a totally separate issue, but until it is, running tests in a virtualenv will fail with stuff like the above. Frankly, I think this particular pep8 rule is stupid and makes the code less-readable, but until pep8 0.6 is the standard used in tools/pip-requires, there's not much I can do about that...

While the rest of the code looks fine, I'd like Rick H. to take a looksie, because I have the feeling that his related_images branch will cause this branch to conflict, and the related_images branch has a number of improvements in it re: how we handle OS API /images keys, image properties, and how the translation from image service dicts to OS API dicts works...

-jay

review: Needs Fixing

« Back to merge proposal