Change in vdsm[master]: python3: lib2to3.fixes.fix_idioms

nsoffer at redhat.com nsoffer at redhat.com
Wed Apr 22 10:33:29 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: python3: lib2to3.fixes.fix_idioms
......................................................................


Patch Set 2: Verified-1

(14 comments)

Please separate the sorted() changes, which are correct, from the type() and isinstance() which are mostly unneeded and some are incorrect.

https://gerrit.ovirt.org/#/c/40125/2/lib/vdsm/ipwrapper.py
File lib/vdsm/ipwrapper.py:

Line 69: 
Line 70: 
Line 71: def equals(cls):
Line 72:     def __eq__(self, other):
Line 73:         return isinstance(other, cls) and self.__dict__ == other.__dict__
This is incorrect - this test must succeeds when object.__class__ are the same.

isinsatnce() is not more pythonic than type(), they are just different, you should use the best for the current use case.
Line 74:     cls.__eq__ = __eq__
Line 75:     return cls
Line 76: 
Line 77: 


https://gerrit.ovirt.org/#/c/40125/2/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 733: def tobool(s):
Line 734:     try:
Line 735:         if s is None:
Line 736:             return False
Line 737:         if isinstance(s, bool):
Why do we like to support subclass of bool? We don't need this complexity.

The real fix is to stop supporting any type here. This should be used only for converting string value to boolean value:

    def tobool(s):
        return s.lower() == 'true'

Passing unrelated types should fail loudly, this is a programmer error.
Line 738:             return s
Line 739:         if s.lower() == 'true':
Line 740:             return True
Line 741:         return bool(int(s))


https://gerrit.ovirt.org/#/c/40125/2/vdsm/API.py
File vdsm/API.py:

Line 188:                     try:
Line 189:                         with open(fname) as f:
Line 190:                             pickledMachineParams = pickle.load(f)
Line 191: 
Line 192:                         if isinstance(pickledMachineParams, dict):
Why do you want to support subclass of dict?
Line 193:                             self.log.debug('loaded pickledMachineParams ' +
Line 194:                                            str(pickledMachineParams))
Line 195:                             self.log.debug('former conf ' + str(vmParams))
Line 196:                             vmParams.update(pickledMachineParams)


https://gerrit.ovirt.org/#/c/40125/2/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 265:             self.log.info('Error finding path for device', exc_info=True)
Line 266:             raise vm.VolumeError(uuid)
Line 267: 
Line 268:     def prepareVolumePath(self, drive, vmId=None):
Line 269:         if isinstance(drive, dict):
I don't want to support subclass of dict here. I don't want to support here even dict - only drive objects.

Please remove this change.
Line 270:             device = drive['device']
Line 271:             # PDIV drive format
Line 272:             if device == 'disk' and isVdsmImage(drive):
Line 273:                 res = self.irs.prepareImage(


https://gerrit.ovirt.org/#/c/40125/2/vdsm/network/api.py
File vdsm/network/api.py:

Line 174
Line 175
Line 176
Line 177
Line 178
This function should not support bool or None input, only strings.


https://gerrit.ovirt.org/#/c/40125/2/vdsm/rpc/process-schema.py
File vdsm/rpc/process-schema.py:

Line 203:         want to link to the element type.
Line 204:         """
Line 205:         ret = []
Line 206:         for i in items:
Line 207:             if isinstance(i, list):
Why do you want to support list subclass?
Line 208:                 i = i[0]
Line 209:             ret.append(i)
Line 210:         return ret
Line 211: 


https://gerrit.ovirt.org/#/c/40125/2/vdsm/storage/dispatcher.py
File vdsm/storage/dispatcher.py:

Line 68:                 ctask = task.Task(id=None, name=name)
Line 69:                 try:
Line 70:                     response = self.STATUS_OK.copy()
Line 71:                     result = ctask.prepare(func, *args, **kwargs)
Line 72:                     if isinstance(result, dict):
The real fix here is to raise if result is not a dict. We should have a simple interface, and should fail loudly if a task fail to implement the simple interface.
Line 73:                         response.update(result)
Line 74:                     return response
Line 75:                 except se.GeneralException as e:
Line 76:                     self.log.error(e.response())


https://gerrit.ovirt.org/#/c/40125/2/vdsm/storage/resourceFactories.py
File vdsm/storage/resourceFactories.py:

Line 134:             # with no other volumes in chain
Line 135:             template = chain[0].volUUID
Line 136:             del chain[:]
Line 137: 
Line 138:         volUUIDChain = sorted([vol.volUUID for vol in chain])
Use generator expression:

    volUUIDChain = sorted(vol.volUUID for vol in chain)
Line 139: 
Line 140:         # Activate all volumes in chain at once.
Line 141:         # We will attempt to activate all volumes again down to the flow with
Line 142:         # no consequence, since they are already active.


https://gerrit.ovirt.org/#/c/40125/2/vdsm/storage/resourceManager.py
File vdsm/storage/resourceManager.py:

Line 67
Line 68
Line 69
Line 70
Line 71
This should have been simple constant

    class LOCK_STATE:
        FREE = "free"
        SHARED = "shared"
        LOCKED = "locked"

The test client code should do is:

    state is LOCK_STATE.FREE

There is no reason to compare strings and types, adding the danger of two "equal" lock state which are not the same object.

The real fix, remove this code, fix the client code.


Line 80:     def __str__(self):
Line 81:         return self.state
Line 82: 
Line 83:     def __eq__(self, x):
Line 84:         if isinstance(x, str):
There is not need to support any other type here.
Line 85:             return self.state == x
Line 86:         if isinstance(x, self):
Line 87:             return x.state == self.state
Line 88: 


Line 99: 
Line 100:     @classmethod
Line 101:     def validate(cls, state):
Line 102:         try:
Line 103:             if not isinstance(getattr(cls, state), str):
I don't see any need to support other types.
Line 104:                 raise ValueError
Line 105:         except:
Line 106:             raise ValueError("invalid lock state %s" % state)
Line 107: 


https://gerrit.ovirt.org/#/c/40125/2/vdsm/storage/storage_mailbox.py
File vdsm/storage/storage_mailbox.py:

Line 69:     return "%x" % n
Line 70: 
Line 71: 
Line 72: def runTask(args):
Line 73:     if isinstance(args, tuple):
We don't need to support tuple subclass
Line 74:         cmd = args[0]
Line 75:         args = args[1:]
Line 76:     else:
Line 77:         cmd = args


https://gerrit.ovirt.org/#/c/40125/2/vdsm/storage/task.py
File vdsm/storage/task.py:

Line 234:     def __str__(self):
Line 235:         return str(self.value)
Line 236: 
Line 237:     def __eq__(self, x):
Line 238:         if isinstance(x, str):
Why do we need to support string subclass?
Line 239:             return self.value == x
Line 240:         if isinstance(x, self):
Line 241:             return x.value == self.value
Line 242:         return False


https://gerrit.ovirt.org/#/c/40125/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1722:                 for ioTune in decStats["ioTune"]:
Line 1723:                     ioTune["ioTune"] = dict((k, utils.convertToStr(v)) for k, v
Line 1724:                                             in ioTune["ioTune"].iteritems())
Line 1725:                 stats[var] = decStats[var]
Line 1726:             elif not isinstance(decStats[var], dict):
Why do we need to support dict subclass?
Line 1727:                 stats[var] = utils.convertToStr(decStats[var])
Line 1728:             elif var in ('network', 'balloonInfo'):
Line 1729:                 stats[var] = decStats[var]
Line 1730:             else:


-- 
To view, visit https://gerrit.ovirt.org/40125
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I543c07a9fefb592dc5f132453643eb8ac0808c63
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list