Saggi Mizrahi has posted comments on this change.
Change subject: assert: Replace assetion with AssertionError ......................................................................
Patch Set 3:
(5 comments)
I'm not going to argue with you about the validity of development assertions. History has proved that they are useful.
The user will never set the optimize flag (and have VDSM supported) we will be the ones changing it if that ever happens.
http://gerrit.ovirt.org/#/c/29307/3/lib/vdsm/libvirtconnection.py File lib/vdsm/libvirtconnection.py:
Line 37: self.__thread = None Line 38: Line 39: def start(self): Line 40: if self.run: Line 41: raise AssertionError("EventLoop is running")
Why do you think RuntimeError it is better?
AssertionError is for assert. Try and run a threading.Thread twice and see that it gives you a RuntimeError. Line 42: self.__thread = threading.Thread(target=self.__run, Line 43: name="libvirtEventLoop") Line 44: self.__thread.setDaemon(True) Line 45: self.run = True
http://gerrit.ovirt.org/#/c/29307/3/vdsm/rpc/BindingXMLRPC.py File vdsm/rpc/BindingXMLRPC.py:
Line 1132: logLevel = logging.TRACE Line 1133: displayArgs = args Line 1134: if f.__name__ == 'vmDesktopLogin': Line 1135: if 'password' in kwargs: Line 1136: raise AssertionError("Unexpected kwarg: password")
How type error is related? It is used when a function cannot handle some ty
Passing a kwarg that doesn't exist to a function that doesn't expect it usually throws a Type Error
In [1]: def foo(a): ...: pass ...:
In [2]: foo(b=1) ------------------------------------------------------------ --------------- TypeError Traceback (most recent call last) <ipython-input-2-494643805add> in <module>() ----> 1 foo(b=1)
TypeError: foo() got an unexpected keyword argument 'b' Line 1137: if len(args) > 3: Line 1138: displayArgs = args[:3] + ('****',) + args[4:] Line 1139: Line 1140: # Logging current call
http://gerrit.ovirt.org/#/c/29307/3/vdsm/rpc/process-schema.py File vdsm/rpc/process-schema.py:
Line 104: Line 105: # Pop a blank line Line 106: line = lines.pop(0) Line 107: if line != '': Line 108: raise AssertionError("Expected empty line: %s" % line)
Leaving the assert means the code will be broken if running with -O (line.p
If we ever run it with -O is during runtime when schema parsing is a performance issue. Since we control the schema in build time we know the input is correct and can assume so. Line 109: Line 110: # Grab the entity description. It might span multiple lines. Line 111: symbol['desc'] = lines.pop(0) Line 112: while (lines[0] != ''):
Line 113: symbol['desc'] += lines.pop(0) Line 114: Line 115: # Pop a blank line Line 116: line = lines.pop(0) Line 117: if line != '':
See above
see above Line 118: raise AssertionError("Expected empty line: %s" % line) Line 119: Line 120: # Populate the rest of the human-readable data. Line 121: # First try to read the parameters/members information. We are finished
http://gerrit.ovirt.org/#/c/29307/3/vdsm/storage/misc.py File vdsm/storage/misc.py:
Line 695: return False Line 696: Line 697: def exit(self): Line 698: with self._cond: Line 699: if not self._busy:
The cost of this check is insignificant, and removing it is too risky.
This is a classic place for assertion. Every sync object worth it's salt does it's assertions optionally. Line 700: raise AssertionError("Attempt to exit a barrier without " Line 701: "entering") Line 702: self._busy = False Line 703: self._cond.notifyAll()