Hi Rick,

Thank you for reviewing the branch. I have incorporated the changes into the branch and please find the desription below.

Kindly have a go at the branch and let me know if you have any comments. Thanks for your time.

Regards,
Sateesh

-----------------------------------------------------------------------------------------------
Changes as per the comments:-
-----------------------------------------------------------------------------------------------

> There are some minor whitespacing issues (arg lists not lining up[1], etc). This
> isn't a huge deal, but any fixes here would improve the code a tad.

>>> Taken care of

* I'm noticing quite a bit of overriding of __getattr__ and __setattr__. In my
  experience, this can lead to pain down the road since any bugs in the
  getattr/setattr overrides tend to manifest themselves in delightfully
  awful/hard to pin-point ways. I can't offer an alternative solution without
  really getting into the details, but, I wanted to raise the concern at
  least.

>>> Removed unnecessary overrides

* I see a bit of Java-ish patterns which could be simplified in Python. For
  example, some classes that only use @classmethods or @staticmethods. Unless
  there is a specific reason to bind this to a class, it's often a good idea
  to leave them as naked functions.

>>> Removed unnecessary @classmethods (Removed from network_utils.py). Retaining only one static method in a class FaultCheckers in error_util.py that is required to group fault checkers in case we add the same for some other API calls.


* Noticed a few overly broad exception handlers which may end up trapping
  unexpected exceptions. It would be better to be as specific as possible, and
  if you're explicitly trying to ignore all exceptions, perhaps log any
  unrecognized exception.

>>> Updated code to catch specific exceptions wherever possible. At some places, where we are catching generic Exceptions, added log statements.


[1] Example:

> 3153	+def get_add_vswitch_port_group_spec(client_factory, vswitch_name,
> 3154	+                                           port_group_name, vlan_id):

Might be better as:

    def get_add_vswitch_port_group_spec(client_factory, vswitch_name,
                                        port_group_name, vlan_id):
>>> Taken care of 

Specifics
=========

> 621	+    if network_ref == None:

When comparing None better to use identity comparison:

  if network_ref is None:

If its None-ness doesn't matter (as is often the case), you can simplify to:

  if not network_ref:

>>> Yes, makes sense. Taken care of.

> 828	+        self.assertTrue(len(instances) == 1)
> 831	+        self.assertTrue(len(instances) == 0)

Better would be:

    self.assertEqual(len(instances), 1)

The fact that it passes both operands to the function means it can generate a
more helpful error message.

>>> Yes, makes sense. Taken care of.

> 628	+                try:
> 1629	+                    _db_content.get("files").remove(file)
> 1630	+                except Exception:
> 1631	+                    pass
> 1632	+

I'm not sure which exception you're guarding against here, IndexError?
It's usually better to be more specific in catching errors, as an example:

  try:
      _db_content.get("files").remove(file)
  except OSError, e:
      # ENOENT is the only acceptable error
      if e.errno != errno.ENOENT:
          raise
>>> Because db_content is a dictionary we are guarding against "KeyError". Taken care of now.

> 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:
        <a bunch of code which can raise>
    except IndexError:
        pass  # this is okay
    except Exception, e:
        logging.error(_('Unexpected exception raised %s') % e)
>>> Makes sense and tried to follow this guideline.

> 1750	+                  "ds": ds,

One space ---> thattaway.
>>> Didn't get it. AFAIK "ds": ds, looks good as per pep8 (white space between ':' and value ('ds' here)).


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

>>> Retaining the older snippet for readability.

> 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
>>> Makes sense. Incorporated.

> 2544	+        # Use this when VMware fixes their faulty wsdl

Per HACKING, please prefix with TODO(sateesh) or FIXME(sateesh).
>>> Added TODO(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:

>>> Taken care of. Removed unnecessary init methods.


    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
>>> Makes sense. Removed these unnecessary overrides. 


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

>>> Now deriving from 'object'. But made retrieveproperties_fault_checker a staticmethod.

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' :)
>>> Makes sense. Changed the code accordingly.


> 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):

>>> Very true. Done.

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

>>> Makes sense. Removed the unnecessary defs.

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

>>> We are mimicing the ESX SOAP calls, where `propSet` is a property of a dataobject. Since VMware uses camel case for naming the properties (in APIs) in general, we adhered to the same.


> 1667	+    def __init__(self):
> 1668	+        pass

Not needed.
>>> Removed.


> 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:
        <chunk of code>
    except KnownException:
        continue

And if you want to pass on all exceptions, adding:

    except Exception, e:
        LOG.exception(e)
        continue
>>> Taken care of now.


> 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"):

>>> Removed the methods from class and they are now naked methods. There was no need of making them part of a class.