[Notes above were cut-off, continues here...] > 2193 + for elem in vswitches: > 2194 + try: > 2195 + for nic_elem in elem.pnic: > 2196 + if str(nic_elem).split('-')[-1].find(vlan_interface) != -1: > 2197 + return elem.name > 2198 + except Exception: > 2199 + pass Same thing, a more specific exception handler would be easier to follow, modify later if needed. If you're want to skip errors (no matter what), it might be better to log unexpectd errors, for example try: except IndexError: pass # this is okay except Exception, e: logging.error(_('Unexpected exception raised %s') % e) > 1750 + "ds": ds, One space ---> thattaway. > 2213 + for pnic in physical_nics: > 2214 + if vlan_interface == pnic.device: > 2215 + return True > 2216 + return False This is fine code, nice and readable. Just as a point of discussion, the `any` builtin could be used as well: return any((vlan_interface == pnic.device) for pnic in physical_nics) Either way is fine, IMHO. > 2318 +try: > 2319 + READ_CHUNKSIZE = client.BaseClient.CHUNKSIZE > 2320 +except: > 2321 + READ_CHUNKSIZE = 65536 Better to qualify the exception handler, something like: try: READ_CHUNKSIZE = client.BaseClient.CHUNKSIZE except AttributeError: READ_CHUNKSIZE = 65536 > 2544 + # Use this when VMware fixes their faulty wsdl Per HACKING, please prefix with TODO(sateesh) or FIXME(sateesh). > 1364 +class VirtualDisk(DataObject): > 1365 + """ > 1366 + Virtual Disk class. Does nothing special except setting > 1367 + __class__.__name__ to 'VirtualDisk'. Refer place where __class__.__name__ > 1368 + is used in the code. > 1369 + """ > 1370 + > 1371 + def __init__(self): > 1372 + DataObject.__init__(self) > 1373 + > 1374 + > 1375 +class VirtualDiskFlatVer2BackingInfo(DataObject): > 1376 + """VirtualDiskFlatVer2BackingInfo class.""" > 1377 + > 1378 + def __init__(self): > 1379 + DataObject.__init__(self) > 1380 + > 1381 + > 1382 +class VirtualLsiLogicController(DataObject): > 1383 + """VirtualLsiLogicController class.""" > 1384 + > 1385 + def __init__(self): > 1386 + DataObject.__init__(self) Since the __init__ methods are just callng up, you can omit them: class VirtualLsiLogicController(DataObject): """VirtualLsiLogicController class.""" pass > 1351 +class DataObject(object): > 1352 + """Data object base class.""" > 1353 + > 1354 + def __init__(self): > 1355 + pass > 1356 + > 1357 + def __getattr__(self, attr): > 1358 + return object.__getattribute__(self, attr) > 1359 + > 1360 + def __setattr__(self, attr, value): > 1361 + object.__setattr__(self, attr, value) Since the __init__ is a no-op, you can just omit it; same for __setattr__. __getattr__ is doing something a little strange, it's calling __getattribute__ instead of __getattr__. If this is desired, it definitely should have a "NOTE(sateesh)" explaining why it's necessary. That said, it doesn't seem necessary, so, like __init__ and __setattr__, I think it can be removed as well: class DataObject(object): """Data object base class.""" pass > 1166 +class FaultCheckers: You probably want to derive from `object` so this is a new-style class: class FaultCheckers(object): > 1173 + @classmethod > 1174 + def retrieveproperties_fault_checker(self, resp_obj): classmethods should use `cls` rather than `self` for first arg. That said, the `self` isn't used by the method, so it would make more sense as a @staticmethod. Since this really just a function (as indicated by staticmethod), you could consider just treating it like a function rather than attaching it to the FaultChecker class. It's perfectly acceptable in Python to have free-floating functions. Of course, this would depend on your future plans for FaultChecker. > 1136 + Exception.__init__(self) > 1484 + ManagedObject.__init__(self, "HostSystem") > 1551 + ManagedObject.__init__(self, "Datacenter") > 1566 + ManagedObject.__init__(self, "Task") ... [more cases] Consider using the super() syntax, if possible. > 1280 + _db_content[table][obj.obj] = obj This is a rather odd looking line. Just sayin' :) > 1283 +def _get_objects(type): `type` is a Python built-in, if possible, it would be better not to bind that to a local variable. Better: def _get_objects(type_): Even Better: def _get_objects(obj_type): > 1291 +class Prop(object): > 1292 + """Property Object base class.""" > 1293 + > 1294 + def __init__(self): > 1295 + self.name = None > 1296 + self.val = None > 1297 + > 1298 + def setVal(self, val): > 1299 + self.val = val > 1300 + > 1301 + def setName(self, name): > 1302 + self.name = name The setVal and setName setters are unecessary. Python has a philosophy of "We are all consenting adults here"[1], meaning you can access those attributes directly. So: my_prop = Prop() Less Ideal: my_prop.setVal(1) Better: my_prop.val = 1 Of course, if you ever have to attach additional functionality to setting the value, you can use object-properties, like: val = property(get_val, set_val, del_val) [1] http://mail.python.org/pipermail/tutor/2003-October/025932.html > 1314 + object.__setattr__(self, 'propSet', []) Per naming conventions, `propSet` might be better as `prop_set`. > 1667 + def __init__(self): > 1668 + pass Not needed. > 1849 + try: > 1850 + obj_ref = obj.obj > 1851 + # This means that we are doing a search for the managed > 1852 + # dataobjects of the type in the inventory > 1853 + if obj_ref == "RootFolder": > 1854 + for mdo_ref in _db_content[type]: > 1855 + mdo = _db_content[type][mdo_ref] > 1856 + # Create a temp Managed object which has the same ref > 1857 + # as the parent object and copies just the properties > 1858 + # asked for. We need .obj along with the propSet of > 1859 + # just the properties asked for > 1860 + temp_mdo = ManagedObject(mdo.objName, mdo.obj) > 1861 + for prop in properties: > 1862 + temp_mdo.set(prop, mdo.get(prop)) > 1863 + lst_ret_objs.append(temp_mdo) > 1864 + else: > 1865 + if obj_ref in _db_content[type]: > 1866 + mdo = _db_content[type][obj_ref] > 1867 + temp_mdo = ManagedObject(mdo.objName, obj_ref) > 1868 + for prop in properties: > 1869 + temp_mdo.set(prop, mdo.get(prop)) > 1870 + lst_ret_objs.append(temp_mdo) > 1871 + except Exception: > 1872 + continue This is a very broad exception handler. Can you distinguish between exceptions that you expect and are willing to explicitly ignore and everything else? Would prefer: try: except KnownException: continue And if you want to pass on all exceptions, adding: except Exception, e: LOG.exception(e) continue > 2150 +class NetworkHelper: New-style class: class NetworkHelper(object): > 2151 + > 2152 + @classmethod > 2153 + def get_network_with_the_name(cls, session, network_name="vmnet0"): All of the @classmethods for NetworkHelper can be staticmethods since `cls` isn't used. This is more specific and makes it easier to refactor later since you're explicitly saying that know dependency exists on the class. Example: @staticmethod def get_network_with_the_name(session, network_name="vmnet0"):